-
Notifications
You must be signed in to change notification settings - Fork 74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
optimize layer storage format for mem size and perf #1877
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1877 +/- ##
==========================================
- Coverage 96.62% 96.51% -0.11%
==========================================
Files 141 139 -2
Lines 27769 26023 -1746
==========================================
- Hits 26832 25117 -1715
+ Misses 937 906 -31
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
if byts is not None: | ||
return s_msgpack.un(byts)[1] | ||
sode = self.layr._getStorNode(buid) | ||
valt = sode.get('valu') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
valt vs valu? just a odd style nit but it seems weird to have a inconsistent label in the codebase
sode.clear() | ||
|
||
if len(tostor) >= 10000: | ||
logger.warning('...syncing 10k nodes') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an incrementing counter would be useful here
tostor = [] | ||
lastbuid = None | ||
|
||
logger.warning(f'Converting layer from v2 to v3 storage: {self.dirn}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we really could do an estimated magnitude based on form counts (but not an absolute count)
# mop up the left overs | ||
if tostor: | ||
self.layrslab.putmulti(tostor, db=self.bybuidv3) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this drop could take a while. worth noting that we're dropping the table
@@ -368,6 +368,7 @@ async def nodesByProp(self, full): | |||
for layr in self.layers: | |||
genr = layr.liftByProp(None, prop.name) | |||
async for node in self._joinStorGenr(layr, genr): | |||
# TODO should these type of filters yield? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no todos in mainline code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err fixmes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a question about how many results one could have that whiff in the continue block or not. that's really the question about yielding here.
@@ -661,35 +661,17 @@ async def test_layer_stortype_merge(self): | |||
nodes = await core.nodes('inet:ipv4=1.2.3.4 [ +#foo.bar=2015 ]') | |||
self.eq((1325376000000, 1420070400001), nodes[0].getTag('foo.bar')) | |||
|
|||
nodeedits = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC this tested applying node edits and showing that the results of applying edits without changes resulted in a empty set of changes. I guess this would be OBE with the sode format change.
@@ -1,1493 +0,0 @@ | |||
import os |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to remove the associated documentation for this tool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so there is additional clean up for doing that and ensuring that we have links back to the the v2.8.0 docs in place
No description provided.