fsrefs: Optimize IO (take 2) - #340
Merged
Merged
Conversation
Contributor
Author
|
Below is debug output for RAM consumption produced using debug patch: fsrefs: timings + RAM usage for 73GB database |
Contributor
Author
|
(+changelog) |
Contributor
Author
|
@jamadden, I believe this patch should be ok to go in. Would you please have a look? |
Contributor
|
I am a bit lost in the figures from the commit message:
Isn't it 5GB ? |
Access objects in the order of their position in file instead of in the order of their OID. This should give dramatical speedup when data are on HDD. For example @perrinjerome reports that on a 73Go database it takes almost 8h to run fsrefs (where on the same database, fstest takes 15 minutes) [1,2]. After the patch fsrefs took ~80 minutes to run on the same database. In other words this is ~ 6x improvement. Fsrefs has no tests. I tested it only lightly via generating a bit corrupt database with deleted referred object(*), and it gives the same output as unmodified fsrefs. oid 0x0 __main__.Object last updated: 1979-01-03 21:00:42.900001, tid=0x285cbacb70a3db3 refers to invalid objects: oid 0x07 missing: '<unknown>' oid 0x07 object creation was undone: '<unknown>' This "take 2" version is derived from zopefoundation#338 and only iterates objects in the order of their in-file position without building complete references graph in-RAM, because that in-RAM graph would consume ~12GB of memory. Added pos2oid in-RAM index also consumes memory: for the 73GB database in question fs._index takes ~700MB, while pos2oid takes ~2GB. In theory it could be less, because we need only array of oid sorted by key(oid)=fs._index[oid]. However array.array does not support sorting, and if we use plain list to keep just []oid, the memory consumption just for that list is ~5GB. Also because list.sort(key=...) internally allocates memory for key array (and list.sort(cmp=...) was removed from Python3), total memory consumption just to produce list of []oid ordered by pos is ~10GB. So without delving into C/Cython and/or manually sorting the array in Python (= slow), using QQBTree seems to be the best out-of-the-box option for oid-by-pos index. [1] https://lab.nexedi.com/nexedi/zodbtools/merge_requests/19#note_129480 [2] https://lab.nexedi.com/nexedi/zodbtools/merge_requests/19#note_129551 (*) test database generated via a bit modified gen_testdata.py from zodbtools: https://lab.nexedi.com/nexedi/zodbtools/blob/v0.0.0.dev8-28-g129afa6/zodbtools/test/gen_testdata.py + ```diff --- a/zodbtools/test/gen_testdata.py +++ b/zodbtools/test/gen_testdata.py @@ -229,7 +229,7 @@ def ext(subj): return {} # delete an object name = random.choice(list(root.keys())) obj = root[name] - root[name] = Object("%s%i*" % (name, i)) +# root[name] = Object("%s%i*" % (name, i)) # NOTE user/ext are kept empty on purpose - to also test this case commit(u"", u"predelete %s" % unpack64(obj._p_oid), {}) ``` /cc @tim-one, @jeremyhylton, @jamadden
Provide changelog entry.
Contributor
Author
|
@perrinjerome, right, that should be 5GB. I've corrected the description. Thanks for spotting this. |
Contributor
Author
|
( force-pushed to correct commit message as well ) |
jamadden
approved these changes
Mar 29, 2021
jamadden
left a comment
jamadden
left a comment
Member
There was a problem hiding this comment.
LGTM. It's a very simple change just to order disk seeking.
I had some very minor comments about the change note and some leftover commented code.
Comment on lines
+11
to
+12
| - Rework `fsrefs` script to work significantly faster by optimizing how it does | ||
| IO. See `PR 340 <https://github.com/zopefoundation/ZODB/pull/340>`. |
Member
There was a problem hiding this comment.
Suggested change
| - Rework `fsrefs` script to work significantly faster by optimizing how it does | |
| IO. See `PR 340 <https://github.com/zopefoundation/ZODB/pull/340>`. | |
| - Rework ``fsrefs`` script to work significantly faster by optimizing how it does | |
| IO. See `PR 340 <https://github.com/zopefoundation/ZODB/pull/340>`_. |
Double-backtictks are literal code; single backticks are the default object reference.
Links need a trailing underscore to be valid.
Contributor
Author
There was a problem hiding this comment.
Thanks for catching this and sorry I did not verify that rst -> html conversion works correctly after my edits. I've integrated your suggestions into this pull-requst.
By the way, now, after verifying rst, I see this:
(neo) (z4-dev) (g.env) kirr@deco:~/src/wendelin/z/ZODB$ rst2html --link-stylesheet CHANGES.rst x.html
CHANGES.rst:3: (WARNING/2) Duplicate explicit target name: "issue 268".
The warning it emits is about the following text where link for issue 286 seems to be incorrect:
- Fix UnboundLocalError when running fsoids.py script.
See `issue 268 <https://github.com/zopefoundation/ZODB/issues/285>`_.
It should be changed to
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -6,7 +6,7 @@
==================
- Fix UnboundLocalError when running fsoids.py script.
- See `issue 268 <https://github.com/zopefoundation/ZODB/issues/285>`_.
+ See `issue 285 <https://github.com/zopefoundation/ZODB/issues/285>`_.
- Rework ``fsrefs`` script to work significantly faster by optimizing how it does
IO. See `PR 340 <https://github.com/zopefoundation/ZODB/pull/340>`_.If it is ok I can do and push this fix to master after merging this PR.
Member
There was a problem hiding this comment.
Sure, though it'd be fine to just include the fix here.
FWIW, I usually use @mgedmin 's restview to verify rendering because it has an option that makes it do what PyPI does, and because it checks the contents of the markup as they will be seen by PyPI — many zopefoundation projects, including ZODB, create this output by concatenating README.rst and CHANGES.rst.
$ restview --long --pypi
Listening on http://localhost:62676/
127.0.0.1 - - [29/Mar/2021 07:26:22] "GET / HTTP/1.1" 200 -
Contributor
Author
There was a problem hiding this comment.
Thanks for restview link. To do the verification I used to do something like
$ python setup.py --long-description |rst2html - x.html
but restview indeed might be more convenient.
Let's have changelog fixup coming as separate patch (out of this PR) not to mix things.
Contributor
Author
There was a problem hiding this comment.
I've fixed "issue 268" in 2798502e. Hope it is ok.
- remove debug prints; - fix link and literal markup in the changelog. Addresses review feedback provided by @jamadden.
Contributor
Author
|
@jamadden, thanks. I've amended the PR with the changes you suggested. |
Contributor
Author
|
Tests are green. @jamadden, should I wait for you to merge this work, or should it be me to merge/apply this by myself? |
Contributor
Author
|
(if by myself - is it ok to "squash and merge" - i.e. to produce only one commit coming out of this PR ?) |
Member
|
The convention in zopefoundation repos is that the author of the PR commits after getting approval and green CI. Squashing the commits is fine if there is no useful data in the intermediate steps; here, I would have kept the revision that uses the in-memory dicts as that is a useful record of something that was tried and didn't work. |
Contributor
Author
|
Thanks, I see. This PR does not have in-RAM dict as intermediate step as I bootstrapped this PR from scratch (i.e. from master) without taking #338 commit history not to create changes noise. On the other hand the commit message links to #338 and explicitly describes what was tried there and not taken here. This way if one investigate through git blame the history of edits, there is backpointer to |
Contributor
Author
navytux
added a commit
to navytux/ZODB
that referenced
this pull request
Oct 29, 2021
to resolve trivial conflict on CHANGES.rst * origin/master: (22 commits) Fix TypeError for fsoids (zopefoundation#351) Fix deprecation warnings occurring on Python 3.10. fix more PY3 incompatibilities in `fsstats` fix Python 3 incompatibility for `fsstats` add `fsdump/fsstats` test fsdump/fsstats improvements - add coverage combine step - first cut moving tests from Travis CI to GitHub Actions - ignore virtualenv artifacts [ci skip] tests: Run race-related tests with high frequency of switches between threads tests: Add test for load vs external invalidation race tests: Add test for open vs invalidation race fixup! doc/requirements: Require pygments < 2.6 on py2 doc/requirements: Require pygments < 2.6 on py2 fixup! buildout: Fix Sphinx install on Python2 buildout: Fix Sphinx install on Python2 Update README.rst Security fix documentation dependencies (zopefoundation#342) changes: Correct link to UnboundLocalError fsoids.py fix fsrefs: Optimize IO (take 2) (zopefoundation#340) ...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Access objects in the order of their position in file instead of in the order
of their OID. This should give dramatical speedup when data are on HDD.
For example @perrinjerome reports that on a 73Go database it takes
almost 8h to run fsrefs (where on the same database, fstest takes 15
minutes) [1,2]. After the patch fsrefs took ~80 minutes to run on the same
database. In other words this is ~ 6x improvement.
Fsrefs has no tests. I tested it only lightly via generating a bit
corrupt database with deleted referred object(*), and it gives the same
output as unmodified fsrefs.
This "take 2" version is derived from #338
and only iterates objects in the order of their in-file position without
building complete references graph in-RAM, because that in-RAM graph would
consume ~12GB of memory.
Added pos2oid in-RAM index also consumes memory: for the 73GB database in
question fs._index takes ~700MB, while pos2oid takes ~2GB. In theory it could be less,
because we need only array of oid sorted by key(oid)=fs._index[oid]. However
array.array does not support sorting, and if we use plain list to keep just
[]oid, the memory consumption just for that list is ~5GB. Also because
list.sort(key=...) internally allocates memory for key array (and
list.sort(cmp=...) was removed from Python3), total memory consumption just to
produce list of []oid ordered by pos is ~10GB.
So without delving into C/Cython and/or manually sorting the array in Python (=
slow), using QQBTree seems to be the best out-of-the-box option for oid-by-pos index.
[1] https://lab.nexedi.com/nexedi/zodbtools/merge_requests/19#note_129480
[2] https://lab.nexedi.com/nexedi/zodbtools/merge_requests/19#note_129551
(*) test database generated via a bit modified gen_testdata.py from
zodbtools:
https://lab.nexedi.com/nexedi/zodbtools/blob/v0.0.0.dev8-28-g129afa6/zodbtools/test/gen_testdata.py
/cc @tim-one, @jeremyhylton, @jamadden
Closes #338.