search

yhirose / cpp-peglib

A single file C++ header-only PEG (Parsing Expression Grammars) library
MIT License
880 stars 112 forks source link

Is WeakHolder necessary? #224

Closed NotAPenguin0 closed 2 years ago

NotAPenguin0 commented 2 years ago

I've been doing some optimization rounds on my own program using cpp-peglib, and found out that parsing is by far the most expensive operation (although this is not surprising). What did surprise me is that the most time consuming part of the parsing is the following line https://github.com/yhirose/cpp-peglib/blob/924588bb6985416413db34c9ba9ddc4709b79f5c/peglib.h#L1465

Since I was curious I changed weak_ to a be a std::shared_ptr and removed all lock() calls, which resulted in a near 2x speedup for parsing all my test cases. What I'm wondering is, is this usage of std::weak_ptr necessary or can it be avoided some other way?

Performance stats gathered on Windows 10 (clang-cl), Ryzen 5 3600.

NotAPenguin0 commented 2 years ago

As a side note, this change doesn't seem to break any of the tests that weren't already broken (see #225 for that). I unfortunately don't have access to a memory leak detector so I can't check for that.

yhirose commented 2 years ago

@NotAPenguin0, thanks for the feedback. WeakHolder is necessary to handle a recursive grammar properly (or 'cyclic dependency') as below.

S <- A
A <~ B
B <- C A

https://stackoverflow.com/questions/27085782/how-to-break-shared-ptr-cyclic-reference-using-weak-ptr

Hope it helps!