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

[WIP] Fix startup speed #60

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

[WIP] Fix startup speed #60

wants to merge 6 commits into from

Conversation

HarrisonGrodin
Copy link
Owner

@HarrisonGrodin HarrisonGrodin commented Dec 13, 2018

Re: #38.

@HarrisonGrodin HarrisonGrodin added the improvement Improvement to existing feature label Dec 13, 2018
@HarrisonGrodin HarrisonGrodin added this to the 0.1 milestone Dec 13, 2018
@HarrisonGrodin
Copy link
Owner Author

According to some very unscientific testing, current change cuts speed (and allocations) down by a factor of around 2. Still a couple of seconds on a fast machine to run rules() initially (hence the WIP), but this seems like something that could/should be resolved in the near future.

cc: @MasonProtter

@MasonProtter
Copy link
Contributor

Exciting!

@MasonProtter
Copy link
Contributor

Wait, is this related to #38? #38 is about compile times, not runtimes. (Runtime speed is important too of course)

@HarrisonGrodin
Copy link
Owner Author

From what I understood, #38 was meant to be about the speed of initially creating the rule set (by calling rules()), which is currently unreasonably slow the first time due to compilation. This leads to extremely slow normalization, even on smaller terms. Note that on subsequent runs, rules() is much faster (~100x - still not good real time, but more palatable), since Julia doesn't need to recompile the code for rules() generation.

@MasonProtter
Copy link
Contributor

Oh I see, I didn’t realize calling rules() was the culprit!

@HarrisonGrodin
Copy link
Owner Author

julia> using Rewrite

julia> @time rules();
  2.365382 seconds (3.59 M allocations: 179.190 MiB, 3.12% gc time)

julia> @time rules();
  0.017669 seconds (21.51 k allocations: 1007.078 KiB)

Seems like it has to be. 😅

It's probably worth it long-term to rethink how rules are actually stored (and how to cache them effectively), since currently we call rules() implicitly with every call to normalize. However, that will probably come alongside a major design overhaul.

@HarrisonGrodin
Copy link
Owner Author

Oh no, there's more!

julia> t = @term(1 - 2/2);
       rs = rules();

julia> @time normalize(t, rs)
  7.992327 seconds (10.88 M allocations: 547.837 MiB, 3.24% gc time)
@term(0)

julia> @time normalize(t, rs)
  0.015746 seconds (79.30 k allocations: 7.676 MiB, 43.38% gc time)
@term(0)

Probably related to #62.

MasonProtter and others added 4 commits December 16, 2018 17:50
@HarrisonGrodin
Copy link
Owner Author

It seems like matching might be the culprit - in hindsight, this makes sense, considering how often it's called and how expensive some of the operations involved are. Currently working to optimize it.

@HarrisonGrodin
Copy link
Owner Author

I've opened #67 separately, since it seems disjoint from these improvements.

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

Successfully merging this pull request may close these issues.

2 participants