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

MPR#6913: add advanced option to tune performance of pattern matching compiler … #1786

Merged
merged 2 commits into from Jun 2, 2018

Conversation

Projects
None yet
5 participants
@dwightguth
Copy link
Contributor

commented May 18, 2018

…in case of exponential blowup

I never got around to submitting this patch upstream at the time I created it because I figured it should probably have some testing associated with it, but I didn't have the time and resources to come up with a strategy for running all the tests at different levels of pattern-match optimization. That said, it seriously decreases the compilation time in pathological cases involving the pattern-matching compiler, on the order of going from several hours to several minutes.

I am in the process of updating our software to the latest version of OCAML to see if it fixes a bug I am dealing with involving Pervasives.at_exit, and I thought it was time to submit this patch upstream in case anyone wants to take it over. If anyone has any resources to add testing for it, that would be welcome, but I still don't have the time to properly add regression testing for it myself. That said, it has been used in production without any issues by us for several years now, so I don't anticipate that there are any bugs to be found.

@gasche

This comment has been minimized.

Copy link
Member

commented May 18, 2018

cc @maranget (and @trefis in padawan position)

@maranget

This comment has been minimized.

Copy link
Contributor

commented May 23, 2018

Hi,

The patch certainly is harmless and has proved to be useful. I think it can be merged, with companion patches to the documentation.

@gasche

This comment has been minimized.

Copy link
Member

commented May 23, 2018

with companion patches to the documentation.

I would suggest to interpret this comment as the following request: @dwightguth, could you update the documentation of the compiler tools' command line options to document your new flag? The files to be updated are the one in the man/ subdirectory (one per tool with the new option), and manual/manual/cmds/unified-options.etex.

@maranget

This comment has been minimized.

Copy link
Contributor

commented May 23, 2018

Thanks Gabriel,

What about the name of the option ? The current name -match-context-rows is fine by me.

@maranget
Copy link
Contributor

left a comment

Approved, up to @gasche opinion on option name.

@gasche

This comment has been minimized.

Copy link
Member

commented May 24, 2018

@dwightguth in the manual, could you maybe mention that this is an advanced option, which should only be tweaked if one observe compilation time blowups with pattern-matching-heavy code of a very particular shape?

The name of the option is fine with me. I like that it starts with -match, which can serve as an umbrella prefix if we decide to give more control over pattern-matching later.

@trefis

This comment has been minimized.

Copy link
Contributor

commented May 24, 2018

I somewhat agree with @gasche, in the sense that the current wording could lead people to think that passing a value higher than 32 will get them better performing matches, and I believe that would be wrong in general.
The purpose of the option is to limit the blow up, and I think we should insist on that.

@stedolan

This comment has been minimized.

Copy link
Contributor

commented May 25, 2018

@dwightguth When you use this option, what lower value do you pass for match-context-rows? Is there a noticable loss in performance from always using the lower value?

I'm wondering whether the issue is just that the constant 32 is too big, and whether using a smaller constant would fix the problem without introducing a new arcane option.

@dwightguth

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2018

Typically I use the value 2. I don't see much of a performance impact from the value 32, but that being said, it's worth noting that my example spends most of its compilation time compiling an enormous machine-generated pattern matching expression with thousands of cases. I suspect the reason why I don't see much difference in performance between 2 and 32 is that a value much higher than 32 would be needed in order to actually generate particularly clever optimization of the match expression. I don't think you can necessarily generalize from there to the assumption that 32 will always be roughly as performant as 2.

@dwightguth dwightguth force-pushed the dwightguth:match-context-rows branch from 5fd4b80 to 0490b23 May 25, 2018

@gasche gasche force-pushed the dwightguth:match-context-rows branch from 0490b23 to 12e9c1d Jun 1, 2018

@gasche

This comment has been minimized.

Copy link
Member

commented Jun 1, 2018

@dwightguth: I pushed a change to your branch that makes the reference manual wording a bit more detailed, and removes the --help wording to just refer to the manual instead (using a neat annotation to get the Manual build to check for consistency of section numbers.) If the change works for you, I'll go ahead and merge.

@dwightguth

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2018

Looks good to me.

@gasche gasche merged commit 5d1022d into ocaml:trunk Jun 2, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Copy link
Member

commented Jun 2, 2018

Merged. Thanks for the patch!

@dwightguth dwightguth deleted the dwightguth:match-context-rows branch Aug 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.