This is an archive of the discontinued Mercurial Phabricator instance.

Differential D630

cstore: extend and refactor deltachain class
ClosedPublic

Authored by ms2316 on Sep 5 2017, 10:40 AM.
Tags
None
Subscribers
None

Details

Reviewers
ryanmce
durham
simonfar
Group Reviewers
Restricted Project
Commits
rFBHGX558fcd49a428: cstore: extend and refactor deltachain class
Summary

Extended DeltaChain with CDeltaChain and PyDeltaChain which are wrappers around
c and python delta chains respectively. The declaration and implementation
of c and python delta chains as well as DeltaChainLink were put in a different
file.

Test Plan
  • Ensure that unit tests pass

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ms2316 created this revision.Sep 5 2017, 10:40 AM
Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 5 2017, 10:40 AM
ryanmce requested changes to this revision.Sep 5 2017, 11:05 AM

You could make this even more trivial to review with with (a) moving the code followed by (b) renaming to CDataPack and introducing the abstract base class, then (c) Adding PythonDataPack. Bonus points for a 4th one that changes code() to status() but I think that might be overkill. When you optimize for reviewer brainpower, you get faster reviews :-)

cstore/deltachain.h
130–131

nit: should have space after //

165

Worth a comment why you're not freeing anything here like you do above. (I think it's because Python is handling it's own memory -- is that correct?)

This revision now requires changes to proceed.Sep 5 2017, 11:05 AM
ms2316 edited edge metadata.Sep 5 2017, 11:25 AM
ms2316 marked 2 inline comments as done.
ms2316 updated this revision to Diff 1613.

Added and edited comments

durham accepted this revision.Sep 5 2017, 12:07 PM
durham added inline comments.Sep 5 2017, 12:48 PM
cstore/deltachain.h
28

filename and deltabasefilename can be different lengths, so we should store the sizes separately.

ms2316 updated this revision to Diff 1615.Sep 5 2017, 3:15 PM

DeltaChainLink now also stores size of deltabasefilename

ms2316 marked an inline comment as done.Sep 6 2017, 5:56 AM
durham accepted this revision.Sep 6 2017, 12:22 PM
ryanmce accepted this revision.Sep 7 2017, 11:03 AM
This revision is now accepted and ready to land.Sep 7 2017, 11:03 AM
Closed by commit rFBHGX558fcd49a428: cstore: extend and refactor deltachain class (authored by ms2316). · Explain WhySep 12 2017, 11:08 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 1754

cstore/datapackstore.cpp

it != _packs.end(); it != _packs.end();
it++) { it++) {
close_datapack(*it); close_datapack(*it);
} }
} }
DeltaChainIterator DatapackStore::getDeltaChain(const Key &key) { DeltaChainIterator DatapackStore::getDeltaChain(const Key &key) {
std::shared_ptr<DeltaChain> chain = this->getDeltaChainRaw(key); std::shared_ptr<DeltaChain> chain = this->getDeltaChainRaw(key);
if (chain->code() == GET_DELTA_CHAIN_OK) { if (chain->status() == GET_DELTA_CHAIN_OK) {
return DeltaChainIterator(chain); return DeltaChainIterator(chain);
} }
throw MissingKeyError("unable to find delta chain"); throw MissingKeyError("unable to find delta chain");
} }
std::shared_ptr<DeltaChain> DatapackStore::getDeltaChainRaw(const Key &key) { std::shared_ptr<DeltaChain> DatapackStore::getDeltaChainRaw(const Key &key) {
for(std::vector<datapack_handle_t*>::iterator it = _packs.begin(); for(std::vector<datapack_handle_t*>::iterator it = _packs.begin();
it != _packs.end(); it != _packs.end();
it++) { it++) {
datapack_handle_t *pack = *it; datapack_handle_t *pack = *it;
delta_chain_t chain = getdeltachain(pack, (const uint8_t *) key.node); delta_chain_t chain = getdeltachain(pack, (const uint8_t *) key.node);
if (chain.code == GET_DELTA_CHAIN_OOM) { if (chain.code == GET_DELTA_CHAIN_OOM) {
throw std::runtime_error("out of memory"); throw std::runtime_error("out of memory");
} else if (chain.code == GET_DELTA_CHAIN_NOT_FOUND) { } else if (chain.code == GET_DELTA_CHAIN_NOT_FOUND) {
freedeltachain(chain); freedeltachain(chain);
continue; continue;
} else if (chain.code != GET_DELTA_CHAIN_OK) { } else if (chain.code != GET_DELTA_CHAIN_OK) {
freedeltachain(chain); freedeltachain(chain);
continue; continue;
} }
// Pass ownership of chain to DeltaChain // Pass ownership of chain to CDeltaChain
return std::make_shared<DeltaChain>(chain); return std::make_shared<CDeltaChain>(chain);
} }
// Check if there are new packs available // Check if there are new packs available
std::vector<datapack_handle_t*> refreshed = refresh(); std::vector<datapack_handle_t*> refreshed = refresh();
for(std::vector<datapack_handle_t*>::iterator it = refreshed.begin(); for(std::vector<datapack_handle_t*>::iterator it = refreshed.begin();
it != refreshed.end(); it != refreshed.end();
it++) { it++) {
datapack_handle_t *pack = *it; datapack_handle_t *pack = *it;
delta_chain_t chain = getdeltachain(pack, (const uint8_t *) key.node); delta_chain_t chain = getdeltachain(pack, (const uint8_t *) key.node);
if (chain.code == GET_DELTA_CHAIN_OOM) { if (chain.code == GET_DELTA_CHAIN_OOM) {
throw std::runtime_error("out of memory"); throw std::runtime_error("out of memory");
} else if (chain.code == GET_DELTA_CHAIN_NOT_FOUND) { } else if (chain.code == GET_DELTA_CHAIN_NOT_FOUND) {
freedeltachain(chain); freedeltachain(chain);
continue; continue;
} else if (chain.code != GET_DELTA_CHAIN_OK) { } else if (chain.code != GET_DELTA_CHAIN_OK) {
freedeltachain(chain); freedeltachain(chain);
continue; continue;
} }
// Pass ownership of chain to DeltaChain // Pass ownership of chain to CDeltaChain
return std::make_shared<DeltaChain>(chain); return std::make_shared<CDeltaChain>(chain);
} }
return std::make_shared<DeltaChain>(GET_DELTA_CHAIN_NOT_FOUND); return std::make_shared<CDeltaChain>(GET_DELTA_CHAIN_NOT_FOUND);
} }
Key *DatapackStoreKeyIterator::next() { Key *DatapackStoreKeyIterator::next() {
Key *key; Key *key;
while ((key = _missing.next()) != NULL) { while ((key = _missing.next()) != NULL) {
if (!_store.contains(*key)) { if (!_store.contains(*key)) {
return key; return key;
} }

cstore/datastore.h

// Copyright (c) 2004-present, Facebook, Inc. // Copyright (c) 2004-present, Facebook, Inc.
// All Rights Reserved. // All Rights Reserved.
// //
// This software may be used and distributed according to the terms of the // This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version. // GNU General Public License version 2 or any later version.
// datastore.h - c++ declarations for a data store // datastore.h - c++ declarations for a data store
// no-check-code // no-check-code
#ifndef FBHGEXT_DATASTORE_H #ifndef FBHGEXT_DATASTORE_H
#define FBHGEXT_DATASTORE_H #define FBHGEXT_DATASTORE_H
extern "C" {
#include "cdatapack/cdatapack.h"
}
#include <memory> #include <memory>
#include <vector>
#include "cstore/deltachain.h"
#include "cstore/key.h" #include "cstore/key.h"
#include "clib/portability/portability.h"
class DeltaChainLink {
private:
const char *_filename, *_deltabasefilename;
const uint8_t *_node, *_deltabasenode, *_delta;
uint16_t _filenamesz;
uint64_t _deltasz;
public:
DeltaChainLink(delta_chain_link_t *link) {
if (link) {
_filename = link->filename;
_deltabasefilename = link->filename;
_node = link->node;
_deltabasenode = link->deltabase_node;
_delta = link->delta;
_filenamesz = link->filename_sz;
_deltasz = link->delta_sz;
} else {
_filename = NULL;
_deltabasefilename = NULL;
_node = NULL;
_deltabasenode = NULL;
_delta = NULL;
_filenamesz = 0;
_deltasz = 0;
}
}
const char* filename() {
return _filename;
}
const char* deltabasefilename() {
return _deltabasefilename;
}
const uint8_t* node() {
return _node;
}
const uint8_t* deltabasenode() {
return _deltabasenode;
}
const uint8_t* delta() {
return _delta;
}
uint16_t filenamesz() {
return _filenamesz;
}
uint64_t deltasz() {
return _deltasz;
}
bool isdone() {
return (_filename == NULL);
}
};
/*
* This class takes ownership of a delta chain
*/
class DeltaChain {
private:
//C DeltaChain
delta_chain_t _chain;
public:
//The constructor does a shallow copy of the delta chain and since the
//ownership is taken by this class it is responsible for memory management
DeltaChain(delta_chain_t chain) : _chain(chain) {}
DeltaChain(get_delta_chain_code_t error) {
_chain = COMPOUND_LITERAL(delta_chain_t) { GET_DELTA_CHAIN_NOT_FOUND };
}
~DeltaChain() {
freedeltachain(_chain);
}
const DeltaChainLink getlink(const size_t idx) {
return DeltaChainLink(&(_chain.delta_chain_links[idx]));
}
size_t linkcount() {
return _chain.links_count;
}
get_delta_chain_code_t code() {
return _chain.code;
}
};
class DeltaChainIterator {
private:
size_t _index;
protected:
std::vector< std::shared_ptr<DeltaChain> > _chains;
DeltaChainIterator() :
_index(0) {}
virtual std::shared_ptr<DeltaChain> getNextChain(const Key &key) {
return std::make_shared<DeltaChain>(GET_DELTA_CHAIN_NOT_FOUND);
}
public:
DeltaChainIterator(std::shared_ptr<DeltaChain> chain) :
_index(0) {
_chains.push_back(chain);
}
DeltaChainLink next() {
std::shared_ptr<DeltaChain> chain = _chains.back();
if (_index >= chain->linkcount()) {
// If we're not at the end, and we have a callback to fetch more, do the
// fetch.
bool refreshed = false;
if (chain->linkcount() > 0) {
DeltaChainLink result = chain->getlink(_index - 1);
const uint8_t *deltabasenode = result.deltabasenode();
if (memcmp(deltabasenode, NULLID, BIN_NODE_SIZE) != 0) {
Key key(result.filename(), result.filenamesz(),
(const char*)deltabasenode, BIN_NODE_SIZE);
std::shared_ptr<DeltaChain> newChain = this->getNextChain(key);
if (newChain->code() == GET_DELTA_CHAIN_OK) {
// Do not free the old chain, since the iterator consumer may
// still be holding references to it.
_chains.push_back(newChain);
chain = _chains.back();
_index = 0;
refreshed = true;
}
}
}
if (!refreshed) {
return DeltaChainLink(NULL);
}
}
DeltaChainLink result = chain->getlink(_index);
_index++;
return result;
}
};
class DataStore { class DataStore {
protected: protected:
DataStore() {} DataStore() {}
public: public:
virtual ~DataStore() {} virtual ~DataStore() {}

cstore/deltachain.h

  • This file was added.
// Copyright (c) 2004-present, Facebook, Inc.
// All Rights Reserved.
//
// This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version.
// deltachain.h - c++ declaration of deltachain and related classes
// no-check-code
#ifndef FBHGEXT_DELTACHAIN_H
#define FBHGEXT_DELTACHAIN_H
extern "C" {
#include "cdatapack/cdatapack.h"
}
#include <memory>
#include <vector>
#include "cstore/key.h"
/*
* Wrapper around delta chain link, both C and Python
*/
class DeltaChainLink {
private:
const char *_filename, *_deltabasefilename;
const uint8_t *_node, *_deltabasenode, *_delta;
durhamUnsubmitted
Done

filename and deltabasefilename can be different lengths, so we should store the sizes separately.

durham: filename and deltabasefilename can be different lengths, so we should store the sizes…
uint16_t _filenamesz, _deltabasefilenamesz;
uint64_t _deltasz;
public:
DeltaChainLink(delta_chain_link_t *link) {
if (link) {
_filename = link->filename;
_deltabasefilename = link->filename;
_node = link->node;
_deltabasenode = link->deltabase_node;
_delta = link->delta;
_filenamesz = link->filename_sz;
_deltabasefilenamesz = link->filename_sz;
_deltasz = link->delta_sz;
} else {
_filename = NULL;
_deltabasefilename = NULL;
_node = NULL;
_deltabasenode = NULL;
_delta = NULL;
_filenamesz = 0;
_deltabasefilenamesz = 0;
_deltasz = 0;
}
}
DeltaChainLink(const char *filename,
const char *deltabasefilename,
const uint8_t *node,
const uint8_t *deltabasenode,
const uint8_t *delta,
uint16_t filenamesz,
uint16_t deltabasefilenamesz,
uint64_t deltasz) :
_filename(filename),
_deltabasefilename(deltabasefilename),
_node(node),
_deltabasenode(deltabasenode),
_delta(delta),
_filenamesz(filenamesz),
_deltabasefilenamesz(deltabasefilenamesz),
_deltasz(deltasz) {}
~DeltaChainLink() = default;
const char* filename() {
return _filename;
}
const char* deltabasefilename() {
return _deltabasefilename;
}
const uint8_t* node() {
return _node;
}
const uint8_t* deltabasenode() {
return _deltabasenode;
}
const uint8_t* delta() {
return _delta;
}
uint16_t filenamesz() {
return _filenamesz;
}
uint16_t deltabasefilenamesz() {
return _deltabasefilenamesz;
}
uint64_t deltasz() {
return _deltasz;
}
bool isdone() {
return (_filename == NULL);
}
};
/*
* Abstract delta chain class
*/
class DeltaChain {
protected:
DeltaChain() {}
public:
virtual ~DeltaChain() {}
virtual const DeltaChainLink getlink(const size_t) = 0;
virtual size_t linkcount() = 0;
virtual get_delta_chain_code_t status() = 0;
};
/*
* Wrapper around C delta chain
* CDeltaChain takes ownership of delta_chain_t
*/
ryanmceUnsubmitted
Done

nit: should have space after //

ryanmce: nit: should have space after //
class CDeltaChain : public DeltaChain {
private:
delta_chain_t _chain;
public:
// The constructor does a shallow copy of the delta chain and since the
// ownership is taken by this class it is responsible for memory management
CDeltaChain(delta_chain_t chain) : _chain(chain) {}
CDeltaChain(get_delta_chain_code_t error) :
_chain(COMPOUND_LITERAL(delta_chain_t) { GET_DELTA_CHAIN_NOT_FOUND }) {}
// Memory of _chain has to be deallocated because it is a C struct that
// contains an array of delta_chain_link_t's
~CDeltaChain() {
freedeltachain(_chain);
}
const DeltaChainLink getlink(const size_t idx) {
return DeltaChainLink(&(_chain.delta_chain_links[idx]));
}
size_t linkcount() {
return _chain.links_count;
}
get_delta_chain_code_t status() {
return _chain.code;
}
};
/*
* Wrapper around python delta chain
*/
ryanmceUnsubmitted
Done

Worth a comment why you're not freeing anything here like you do above. (I think it's because Python is handling it's own memory -- is that correct?)

ryanmce: Worth a comment why you're not freeing anything here like you do above. (I think it's because…
class PyDeltaChain : public DeltaChain {
private:
std::shared_ptr< std::vector<DeltaChainLink> > _chain;
public:
PyDeltaChain(std::shared_ptr< std::vector<DeltaChainLink> > chain) :
_chain(chain) {}
// Default destructor is used, because the destructor of _chain object
// will free the allocated memory automatically.
~PyDeltaChain() {}
const DeltaChainLink getlink(const size_t idx) {
return _chain->at(idx);
}
size_t linkcount() {
return _chain->size();
}
get_delta_chain_code_t status() {
if (_chain->size()) {
return GET_DELTA_CHAIN_OK;
} else {
return GET_DELTA_CHAIN_NOT_FOUND;
}
}
};
class DeltaChainIterator {
private:
size_t _index;
protected:
std::vector< std::shared_ptr<DeltaChain> > _chains;
DeltaChainIterator() :
_index(0) {}
virtual std::shared_ptr<DeltaChain> getNextChain(const Key &key) {
return std::make_shared<CDeltaChain>(GET_DELTA_CHAIN_NOT_FOUND);
}
public:
DeltaChainIterator(std::shared_ptr<DeltaChain> chain) :
_index(0) {
_chains.push_back(chain);
}
DeltaChainLink next();
};
#endif // FBHGEXT_DELTACHAIN_H

cstore/deltachain.cpp

  • This file was added.
// Copyright (c) 2004-present, Facebook, Inc.
// All Rights Reserved.
//
// This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version.
// deltachain.h - c++ implementation of deltachain and related classes
// no-check-code
#include "cstore/deltachain.h"
DeltaChainLink DeltaChainIterator::next() {
std::shared_ptr<DeltaChain> chain = _chains.back();
if (_index >= chain->linkcount()) {
// If we're not at the end, and we have a callback to fetch more, do the
// fetch.
bool refreshed = false;
if (chain->linkcount() > 0) {
DeltaChainLink result = chain->getlink(_index - 1);
const uint8_t *deltabasenode = result.deltabasenode();
if (memcmp(deltabasenode, NULLID, BIN_NODE_SIZE) != 0) {
Key key(result.filename(), result.filenamesz(),
(const char*)deltabasenode, BIN_NODE_SIZE);
std::shared_ptr<DeltaChain> newChain = this->getNextChain(key);
if (newChain->status() == GET_DELTA_CHAIN_OK) {
// Do not free the old chain, since the iterator consumer may
// still be holding references to it.
_chains.push_back(newChain);
chain = _chains.back();
_index = 0;
refreshed = true;
}
}
}
if (!refreshed) {
return DeltaChainLink(NULL);
}
}
DeltaChainLink result = chain->getlink(_index);
_index++;
return result;
}

cstore/uniondatapackstore.cpp

std::shared_ptr<DeltaChain> UnionDeltaChainIterator::getNextChain(const Key &key) { std::shared_ptr<DeltaChain> UnionDeltaChainIterator::getNextChain(const Key &key) {
for(std::vector<DataStore*>::iterator it = _store._stores.begin(); for(std::vector<DataStore*>::iterator it = _store._stores.begin();
it != _store._stores.end(); it != _store._stores.end();
it++) { it++) {
DataStore *substore = *it; DataStore *substore = *it;
std::shared_ptr<DeltaChain> chain = substore->getDeltaChainRaw(key); std::shared_ptr<DeltaChain> chain = substore->getDeltaChainRaw(key);
if (chain->code() == GET_DELTA_CHAIN_OK) { if (chain->status() == GET_DELTA_CHAIN_OK) {
return chain; return chain;
} }
} }
throw MissingKeyError("unable to find delta chain"); throw MissingKeyError("unable to find delta chain");
} }
UnionDeltaChainIterator UnionDatapackStore::getDeltaChain(const Key &key) { UnionDeltaChainIterator UnionDatapackStore::getDeltaChain(const Key &key) {

setup.py

], ],
} }
else: else:
availableextmodules = { availableextmodules = {
'cstore' : [ 'cstore' : [
Extension('cstore', Extension('cstore',
sources=[ sources=[
'cstore/datapackstore.cpp', 'cstore/datapackstore.cpp',
'cstore/deltachain.cpp',
'cstore/py-cstore.cpp', 'cstore/py-cstore.cpp',
'cstore/pythonutil.cpp', 'cstore/pythonutil.cpp',
'cstore/uniondatapackstore.cpp', 'cstore/uniondatapackstore.cpp',
'ctreemanifest/manifest.cpp', 'ctreemanifest/manifest.cpp',
'ctreemanifest/manifest_entry.cpp', 'ctreemanifest/manifest_entry.cpp',
'ctreemanifest/manifest_fetcher.cpp', 'ctreemanifest/manifest_fetcher.cpp',
'ctreemanifest/manifest_ptr.cpp', 'ctreemanifest/manifest_ptr.cpp',
'ctreemanifest/treemanifest.cpp', 'ctreemanifest/treemanifest.cpp',

tests/test-check-code-hg.t

Skipping clib/null_test.c it has no-che?k-code (glob) Skipping clib/null_test.c it has no-che?k-code (glob)
Skipping clib/portability/inet.h it has no-che?k-code (glob) Skipping clib/portability/inet.h it has no-che?k-code (glob)
Skipping clib/portability/portability.h it has no-che?k-code (glob) Skipping clib/portability/portability.h it has no-che?k-code (glob)
Skipping clib/portability/unistd.h it has no-che?k-code (glob) Skipping clib/portability/unistd.h it has no-che?k-code (glob)
Skipping clib/sha1.h it has no-che?k-code (glob) Skipping clib/sha1.h it has no-che?k-code (glob)
Skipping cstore/datapackstore.cpp it has no-che?k-code (glob) Skipping cstore/datapackstore.cpp it has no-che?k-code (glob)
Skipping cstore/datapackstore.h it has no-che?k-code (glob) Skipping cstore/datapackstore.h it has no-che?k-code (glob)
Skipping cstore/datastore.h it has no-che?k-code (glob) Skipping cstore/datastore.h it has no-che?k-code (glob)
Skipping cstore/deltachain.cpp it has no-che?k-code (glob)
Skipping cstore/deltachain.h it has no-che?k-code (glob)
Skipping cstore/key.h it has no-che?k-code (glob) Skipping cstore/key.h it has no-che?k-code (glob)
Skipping cstore/match.h it has no-che?k-code (glob) Skipping cstore/match.h it has no-che?k-code (glob)
Skipping cstore/py-cdatapack.h it has no-che?k-code (glob) Skipping cstore/py-cdatapack.h it has no-che?k-code (glob)
Skipping cstore/py-cstore.cpp it has no-che?k-code (glob) Skipping cstore/py-cstore.cpp it has no-che?k-code (glob)
Skipping cstore/py-datapackstore.h it has no-che?k-code (glob) Skipping cstore/py-datapackstore.h it has no-che?k-code (glob)
Skipping cstore/py-structs.h it has no-che?k-code (glob) Skipping cstore/py-structs.h it has no-che?k-code (glob)
Skipping cstore/py-treemanifest.h it has no-che?k-code (glob) Skipping cstore/py-treemanifest.h it has no-che?k-code (glob)
Skipping cstore/pythonkeyiterator.h it has no-che?k-code (glob) Skipping cstore/pythonkeyiterator.h it has no-che?k-code (glob)