-
Notifications
You must be signed in to change notification settings - Fork 192
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
This is a new attempt to fix the add performance #83
Conversation
I just looked at the new code and I like it since it is much simpler than the old code. But I have two questions about why we are doing this at all? Making an index of the node-id seems unnecessary, since we can surely just ask neo4j directly if a node with a specific id exists, right and then check if it is a geometry node. That should be faster than using an index, and faster than the old way of iterating a long chain. |
Having just read the discussion between Axel and Jonathan on the neo4j forum, I guess perhaps my comments are not competely valid. I made a different comment in the forum. |
Very cool! Do we have any test inserting a sizable amount of geometries to test the better performance? |
Yes, there's testAddPerformance() in IndexProviderTest. When considering to merge this, please look at Jonathan's suggestions regarding index name and deletion in the forum. I can provide an additional PR tonight for that. |
Axel, On Mon, Jan 7, 2013 at 10:04 AM, amorgner notifications@github.com wrote:
|
Interesting ... I can't get the testAddPerformance() to pass on my Mac (MPB 8G 2.4GHz SSD). There's a performance drop after abt 14,000 nodes:
Same code, tested at my dev box (Linux 3.2.0, Core i7, 8 GB, SSD):
No time-consuming jobs were running on either system, enough free RAM, no swapping. The first 7,000 nodes are inserted at nearly equal performance, but after that, the Mac drops off. Any ideas? |
That is very interesting. Any chance to profile that? On Mon, Jan 7, 2013 at 9:07 PM, amorgner notifications@github.com wrote:
|
… only 403 replies from there
Got it. Tests failed because of too small heap size. Will add an appropriate section to the pom.xml to make sure tests are running with enough heap space. |
…with 1G heap space)
…ex when they are removed from layer index
Now the test passes on Travis. Not sure how to proceed here. Please up-/downvote if you are pro/con merging this PR. |
What is the current performance profile? No degradation anymore? If not, go ahead! |
Btw, awesome to get i building on travis, good work! You should announce it on the list or in a blog when merged. |
Performance profile is linear (with some GC pauses depending on tx size and heap size, nothing unusual). Speed is about 1,250 nodes/s on a Core i7 |
Seems like a huge performance improvement. I would love to see a comparison of the before and after scenario, so we can really see the difference. I actually tried to do this yesterday, but ran out of time before I could merge your pull requests into my local devenv. I was planning to merge, commit, push, but also think it would be better if you did it. If all test cases pass (including old ones), then I'm happy that you merge this. |
The new approach uses a classic index which is a better solution for id lookups IMHO.
Details of the issue and the discussion see here
#72
and here
https://groups.google.com/forum/#!msg/neo4j/qgGI60taSmA/POIR3UGks0kJ