This is an archive of the discontinued Mercurial Phabricator instance.

cstore: uniondatapackstore support for python stores
AbandonedPublic

Authored by ms2316 on Sep 1 2017, 3:58 PM.
Tags
None
Subscribers
None

Details

Reviewers
durham
simonfar
ryanmce
Group Reviewers
Restricted Project
Summary

Now uniondatapackstore can also hold python data stores. PythonDataStore
wrapper simply passes function calls to underlying python objects and marshals
the output.

Test Plan
  • Ensure unit tests pass

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Branch
pythonstore (bookmark) on default (branch)
Lint
Lint OK
Unit
Unit Tests OK

Event Timeline

ms2316 created this revision.Sep 1 2017, 3:58 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 1 2017, 3:58 PM
ms2316 added a comment.Sep 1 2017, 4:12 PM

And testing plan is not exhaustive of course.

cstore/datastore.h
16

I think that looks ugly. Perhaps we should make a BaseDeltaChain class and extend CDeltaChain and PythonDeltaChain from it?

cstore/py-pythondatastore.h
2

This of course is not a header file. But, I am trying to separate python specific code from non python specific code, because of 'Eden related stuff'.

cstore/uniondatapackstore.cpp
34

I dont like it either because it violates Liskov substitution principle, but we need to do memory management for pythondatastores. Perhaps there is a better way?

durham edited edge metadata.Sep 1 2017, 6:05 PM

I only read part of it. Will add more comments later.

cstore/datastore.h
16

Hmm, yea, given all the if statements I think that makes sense.

23

Does this work? How does sizeof know the size of filename? The only way I can think of is by walking until the first \0 character, which won't work for delta. My guess is that sizeof does a compile time lookup of the size of the compile time type, and therefore doesn't actually return the string length. I would pass the size's in directly.

cstore/uniondatapackstore.cpp
34

UnionDatapackStore doesn't own the lifetime of normal cdatapack stores because they were created initially as a Python object and and therefore kept alive by a reference from py_uniondatapackstore. Maybe we just follow the same pattern and add a std::vector<PythonObj> pythonsubstores member next to substores on py_uniondatapackstore in py-structs.h. So they'll have the same lifetime structure and we can get rid of this code here.

ms2316 marked 3 inline comments as done.Sep 4 2017, 12:27 PM
ms2316 updated this revision to Diff 1598.

Extended CDeltaChain and PyDeltaChain from base DeltaChain. Updated memory management for PythonDataStores.
Pass filename and delta size directly to DeltaChainLink costructor. Refactored the code.

ms2316 updated this revision to Diff 1600.Sep 4 2017, 12:33 PM

Removed redundant include

ms2316 marked 2 inline comments as done.Sep 4 2017, 1:35 PM
ms2316 added inline comments.
cstore/datastore.h
23

My mistake. Forgot how sizeof works.

cstore/py-structs.h
21

By the way, I read here that semicolon is not needed as PyObject_HEAD is a macro that already contains semicolon. And that the behavior is implementation dependent if the semicolon is added.

ryanmce requested changes to this revision.Sep 5 2017, 9:40 AM

Needs to be split into at least three -- two refactors and then the code you're actually adding. A few inline comments. Will review more in depth after you've split it up.

cstore/datastore.h
21

Split this refactor into a smaller commit below this one. Try to avoid refactors combined with new code -- it makes everything much harder to review.

cstore/py-datapackstore.h
112–140

Should be split into a separate commit to make this one more reviewable. The first commit can move this and show everything still works, then future commits can implement the new/additional functionality.

218

I'd name this better -- perhaps is_datapack?

220

No need to do == 1 here, unless IsInstance can return something other than 1 and 0, in which case this would warrant a comment.

This revision now requires changes to proceed.Sep 5 2017, 9:40 AM
ms2316 added inline comments.Sep 5 2017, 9:54 AM
cstore/py-datapackstore.h
220

In fact yes, it also can return -1 in case of an error, which I missed

ms2316 abandoned this revision.Sep 5 2017, 10:54 AM

D629 D630 D631 replace this diff