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

Replace Mutex with RwLock in string interning code. #162

Merged
merged 1 commit into from
Jan 20, 2022

Conversation

mmp
Copy link
Contributor

@mmp mmp commented Jan 20, 2022

When calling into egg from multiple threads, the Mutex used for STRINGS can become a bottleneck. (In my use case, I have a few hundred expressions that I'd like egg to simplify, so I do that using multiple threads.) In that context, I observed that egg::util::intern was a significant fraction of total runtime and that more time was spent in it than in any of the rest of egg's code(!). Here is what perf on Linux reported:

Screen Shot 2022-01-20 at 12 04 32 PM

(Note that is in the context of a larger application; just over half of total runtime is in egg but that 43% is with respect to total runtime.) Total runtime for my test is 4.04s user, 14.22s sys.

This PR replaces the Mutex is replaced with a reader-writer lock. perf reports roughly a 2x speedup:

Screen Shot 2022-01-20 at 12 12 45 PM

which is also reflected in the total runtime for my system going to 9.26s user, 0.21s sys.

When calling into egg from multiple threads, the Mutex used for STRINGS can
become a bottleneck.  For the application where I noticed this, replacing
the Mutex with a reader-writer lock cuts time spent in egg::util::intern by
half.
@mwillsey
Copy link
Member

Great PR, I will accept after CI goes.

Out of curiosity, what were you doing that caused the interning to get called so frequently? My initial intention was that interning should only be called while setting things up.

@mmp
Copy link
Contributor Author

mmp commented Jan 20, 2022

To make a long story short, most of my code is C++ and so I'm going through the FFI to call into egg. Currently, that involves converting my expressions into strings, passing them over, simplifying them with egg, and then going back to strings (that I parse back into my own expression representation). That is admittedly not the best approach, but it was an easy way to experiment with egg before going further...

(Related, I'm seeing a decent amount of time spent in parse_sexp_into(), but I have no doubt that is my fault. :-) )

So, I can certainly imagine that the way I'm using egg is part of the problem...

@mwillsey
Copy link
Member

Ahh, that totally makes sense. If you are crossing FFI using strings, then you don't have much of a choice to parse!

@mwillsey mwillsey merged commit 6353a6b into egraphs-good:main Jan 20, 2022
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.

2 participants