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

eliminating some weak array creations #15

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

nilsbecker
Copy link

i noticed that my simulation-minibenchmarks spent most of their time in weak array creation and garbage collection. here is a naive attempt at reducing this load.

the result is that overhanging weak arrays stay around but will be collected whenever it's convenient. it seems that within react.ml, having weak arrays longer than Wa.len says, is not a problem. Wa.t is not public so there should not be a problem for clients

the tests in test.ml pass. also, in one mini-benchmark,
https://gist.github.com/nilsbecker/2ec8d0a136e2fcc20184#file-react_minibenchmark-ml
this version is up to 20% faster

i am not sure if the Wa.clear is needed on finish. anyway, the whole thing is pretty much a stab in the dark, so could be nonsense.

the idea was to save on gc load by letting weak arrays expire by themselves

test.ml passes. the non-emptyness of the arrays above Wa.len does not seem to be a problem
@dbuenzli
Copy link
Owner

Will need to have a closer look at this but don't have the time at the moment. But my first gut reaction is that this may have a bad impact when used with js_of_ocaml where weak arrays are not. Still it's nice to have isolated that bottleneck but I will have to see if you are not dealing with a pathological case.

@nilsbecker
Copy link
Author

sure. my use case is probably relatively extreme since it measures almost only overhead from mini-events. but maybe others have similar use cases? have not tried any js version

@dbuenzli
Copy link
Owner

As you can see Wa.clear is used in other cases (e.g. node stops) where its important to keep that behaviour for the js backend, so that we leak less. But I think that what you propose is ok for the update step, since the whole step datastructure should be quickly gc'd. Could you maybe try to change your patch to only affect this line and see if you get similar speedups.

@nilsbecker
Copy link
Author

i tried. changing only finish in Step.execute while leaving Wa.clear untouched does not give a noticeable speedup.

@nilsbecker
Copy link
Author

however, the other way around, changing only Wa.clear does speed it up the same, and test.ml still succeeds.

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