Skip to content
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

Improved findrefs execution speed after it runs the first time #114

Merged
merged 3 commits into from
Jul 25, 2017

Conversation

mmarchini
Copy link
Contributor

With those changes, llscan will keep a cache of searched references and
will use this cache to avoid rescanning the entire heap each time.
This will improve performance when executing findrefs multiple times.
Fixes: #97

@mmarchini
Copy link
Contributor Author

I realized that this solution is very memory consuming, so I'll update my PR tomorrow with a much less consuming version, and also with improvements on the cache initialization.

@brendangregg
Copy link
Contributor

I tested the first version, which works, thanks.

Minor comment: I think the whitespace style is inconsistent with the rest of llnode. eg: if(references==nullptr) { should be if (references == nullptr) {.

@indutny
Copy link
Member

indutny commented Jul 10, 2017

make format should be able to fix style nits.

@hhellyer
Copy link
Collaborator

Looks good, I'm happy to do a more detailed review once you've pushed your other improvements.

With those changes, llscan will keep a cache of scanned references and
will use this to avoid rescanning the entire heap each time. This will
hugely improve performance when executing findrefs multiple times.
Important: it will create the entire cache after running findrefs
the first time for each type of scan (by reference, by property and
by string).
Fixes: nodejs#97
@mmarchini mmarchini force-pushed the improve-findrefs-performance branch from 8ad392f to 21d4ce5 Compare July 11, 2017 22:12
@mmarchini
Copy link
Contributor Author

mmarchini commented Jul 11, 2017

Just updated the PR with a new solution, very lightweight on memory and still very fast. It also adds almost no overhead to the first execution.

And I profiled both versions to compare speed using a core with ~180Mb:

Findrefs execution time on master:
findrefs execution time on master

Findrefs execution time with those changes:
findrefs execution time with those changes

What do you guys think?

@mmarchini
Copy link
Contributor Author

Forgot to mention: it will create the entire cache after running findrefs the first time for each type of scan (by reference, by property and by string).

@hhellyer
Copy link
Collaborator

@mmarchini - I think you still have a memory leak when the user changes targets. When you clear the maps you remove the pointers they contain not the memory pointed to by those pointers (created by new ReferencesVector).
You can look at the function LLScan::ClearMapsToInstances() at the bottom of llscan.cc as an example of how to free them, except it turns out that LLScan::ClearMapsToInstances() isn't being called and we have a bit of a memory leak there. I'll raise a separate issue for that!
(As an alternative you could also probably store std::shared_ptr<ReferencesVector>s in your maps, that might nice in C++ terms although there will be a small footprint overhead to using shared pointers as they have to store a reference count.)

There are also a few compiler warnings introduced by the functions AreReferencesLoaded() and GetReferences() where the base implementations don't return anything, at least on Mac, could you tidy those up?

Other than that it all looks good to me and should make life easier when running multiple queries against a dump.

@mmarchini
Copy link
Contributor Author

I think std::shared_ptr will add unnecessary overhead on both speed and memory usage, so I ended up destroying those references manually. I took the liberty of fixing mapsofinstances_ memory leaks too (if you prefere I'll create another PR for that).

Those compiler warnings are not appearing for me (I'm on Ubuntu 16.04), but I think they're fixed now.

@hhellyer hhellyer self-requested a review July 12, 2017 14:11
hhellyer

This comment was marked as off-topic.

@hhellyer hhellyer merged commit 9cabf0b into nodejs:master Jul 25, 2017
@hhellyer
Copy link
Collaborator

Merged, sorry for the delay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants