-
Notifications
You must be signed in to change notification settings - Fork 0
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
Track max level more lazily #119
base: lh/anew-dev-2
Are you sure you want to change the base?
Conversation
Benchmark Results
Benchmark PlotsA plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR. |
Lazy is not always faster. In fact it almost always comes with some overhead (i.e. making something lazy makes it slower when the work still needs to get done). In this case the overhead is on every call to Looking at our worst cases of 2', 4', and 5b', this work already needs to get done and so this is a slight regression. The best case improvement for this laziness is hit by pathological 2 and pathological 4 (repeated long distance shifts in Sampling runtime is slightly up (though that could be just noise). |
I agree, but I changed some parts and now the pathological are all a bit faster/equal in all cases it seems, so to me it seems worth it (even though delete + rand seems 2-3% slower), because I would like to bring down our pathological cases the most. But there is a bug I don't really understand where it comes from (which could even somewhat influence performance). Do you have ideas? (edit:solved) (actually the pathological benchmarks seem a bit flacky) |
ok, solved the bug. Everything seems as fast or faster than before except delete+rand which is 2% slower. I think the trade-off is good and I think this PR is therefore mergeable. |
Ei @LilithHafner, can you give your opinion on this? It should be fine in my opinion because we don't have measurable regressions on any pathological case, and the time for the simplest cases approaches with this change the time of the main branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still benchmarks as a (slight) regression in our worst case. And I'm not particularly concerned about the best cases.
Could you share a plausible use case that has a significant improvement?
I guess I'm just not that enthusiastic about adding complexity to the code and to the invariants unless there's a compelling performance improvement.
function verify_weights(w::DynamicDiscreteSamplers.Weights) | ||
m = w.m |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change seems unrelated and I'd rather not make it because it will make throwing in verify(m)
calls into src
internals harder which will reduce its utility in debugging
This mysteriously fails at the moment but this should be faster