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

type_cases: always propagate #1747

Merged
merged 3 commits into from May 4, 2018
Merged

type_cases: always propagate #1747

merged 3 commits into from May 4, 2018

Conversation

trefis
Copy link
Contributor

@trefis trefis commented Apr 30, 2018

Reading MPR#6542 it seems like 8db1b59 brings a noticeable speedup when typechecking classes, at the cost of make the code of type_cases noticeably more convoluted.

Looking at the code, the following can be deduced:

  • the optimization triggers (i.e. propagate = false) iff. ty_arg is generic and the matching only has one clause, consisting of a variable
  • the optimization avoids:
    • bringing the type of every subpattern to the level of the match (done by a call to unify_var with a fresh variable for every subpattern)
    • generalizing ty_arg'
    • generalizing the type of every subpattern

However, given the first condition, the calls to generalize that are avoided should actually be noops even if we ran them.
So the speed up comes from avoiding the call to unify_var (which is what's hinted at in the MPR, even though the code was slightly different at the time).

As it turns out, the same speed-up can be achieved by a more straightforward change: passing the arguments in the correct order when calling unify_var. Indeed, with the way it is currently called in trunk, it always degrades to unify.

For benchmarking I reused the same test case as given in the MPR, that is:

(echo "class foo = object" ; for ((i = 0; i < 2000; i++)) do echo method x$i = $i; done ; echo end) > test_propagate.ml

I timed the compilation of that file (and a slightly bigger one where I have 8k methods instead of 2k) a few times, with various compilers, here are the results:

TRUNK:

test_propagate 2k methods : ../ocaml-install/bin/ocamlc -c ./test_propagate.ml   0.89s user 0.05s system 99% cpu  0.942 total
test_propagate 8k methods : ../ocaml-install/bin/ocamlc -c ./test_propagate.ml  14.51s user 0.09s system 99% cpu 14.645 total
test_propagate 8k methods : ../ocaml-install/bin/ocamlc -c ./test_propagate.ml  14.36s user 0.12s system 99% cpu 14.492 total
test_propagate 8k methods : ../ocaml-install/bin/ocamlc -c ./test_propagate.ml  14.21s user 0.09s system 99% cpu 14.308 total

UNIFY_VAR in the right order:

test_propagate 2k methods : ../ocaml-install/bin/ocamlc -c ./test_propagate.ml   0.88s user 0.02s system 99% cpu  0.900 total
test_propagate 8k methods : ../ocaml-install/bin/ocamlc -c ./test_propagate.ml  14.09s user 0.10s system 99% cpu 14.203 total
test_propagate 8k methods : ../ocaml-install/bin/ocamlc -c ./test_propagate.ml  14.81s user 0.09s system 99% cpu 14.917 total
test_propagate 8k methods : ../ocaml-install/bin/ocamlc -c ./test_propagate.ml  14.17s user 0.07s system 99% cpu 14.254 total

PROPAGATE = true always:

test_propagate 2k methods : ../ocaml-install/bin/ocamlc -c ./test_propagate.ml   1.39s user 0.05s system 99% cpu  1.445 total
test_propagate 8k methods : ../ocaml-install/bin/ocamlc -c ./test_propagate.ml  31.54s user 0.10s system 99% cpu 31.647 total

PROPAGATE = true always && UNIFY_VAR in the right order:

test_propagate 2k methods : ../ocaml-install/bin/ocamlc -c ./test_propagate.ml   0.80s user 0.02s system 99% cpu  0.822 total
test_propagate 8k methods : ../ocaml-install/bin/ocamlc -c ./test_propagate.ml  14.61s user 0.07s system 99% cpu 14.693 total
test_propagate 8k methods : ../ocaml-install/bin/ocamlc -c ./test_propagate.ml  14.32s user 0.07s system 99% cpu 14.409 total
test_propagate 8k methods : ../ocaml-install/bin/ocamlc -c ./test_propagate.ml  14.45s user 0.07s system 99% cpu 14.544 total

@trefis
Copy link
Contributor Author

trefis commented Apr 30, 2018

For the record, I checked all the other uses of unify_var in the code base: they all pass the arguments in the correct order, though perhaps it would be worth changing unify_var so it takes a named parameter.

@trefis trefis requested a review from garrigue April 30, 2018 16:55
Copy link
Contributor

@garrigue garrigue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well spotted. I didn't expect the cost to come from this single call to unify_var.
If you tested properly on the example of MPR#6542, then this should be sufficient, as this was only an optimization to work around that problem.

@garrigue
Copy link
Contributor

garrigue commented May 4, 2018

Also, looking more carefully at unify and unify_var (properly called), the only difference seems to be the call to occur_univar. I always knew that it was costly, but never went around to find something faster. And Didier Remy as some ideas to make occur itself faster.

@trefis
Copy link
Contributor Author

trefis commented May 4, 2018

Thanks for the review!

@trefis trefis merged commit b864569 into ocaml:trunk May 4, 2018
@trefis trefis deleted the always-propagate branch May 4, 2018 12:33
EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024
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.

None yet

2 participants