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

Major optimisations: 2x faster! #1

Merged
merged 7 commits into from Mar 25, 2016
Merged

Conversation

dolmen
Copy link
Contributor

@dolmen dolmen commented Feb 27, 2016

While profiling Dist::Zilla with Devel::NYTProf I noticed that Data::OptList is heavily used. For example when building the Data-OptList distribution, mkopt is called 2704 times (from Moose and Sub::Exporter) and mkopt_hash 237 times (called by Sub::Exporter).

So here is a set of MAJOR optimisations:

  • the heart of the mkopt main loop is reworked to reduce redundant paths
  • the $must_be parameter is compiled into a closure (this benefits to Sub::Exporter)
  • more tests are added for improved code paths coverage

This will give a boost to any Moose application!

Some statistics (second line is with my optimisations):

mkopt calls mkopt inclusive time mkopt_hash inclusive time benchmark command (in the Data-OptList repo)
2704 91.3ms 32.8ms perl -d:NYTProf -S dzil test
2704 52.8ms 31.3ms PERL5LIB="$PWD/lib" perl -d:NYTProf -ME='$Data::OptList::VERSION="0.110"' -S dzil test

Cc: @karenetheridge @dagolden @kentfredric @autarch @doy @sartak @shadowcat-mst

- avoid assigning undef to $value that is already undef
- reorganize code paths to avoid checking the definedness of $value when
  we know we haven't set the value
Add more tests. The change in coverage is not visible on the
Devel::Cover report for the current code. But that will change with the
refactoring to come...
The __is_a sub is replaced by a closure built from the $must_be
specification.
The code is slightly slower for the scalar $must_be case (as we now
transform that case in the array case with a single element), but
simpler to read/maintain as closures are built in a single place.
The must_be compiler benefits to Sub::Exporter.
@dolmen
Copy link
Contributor Author

dolmen commented Mar 20, 2016

@rjbs Ping!

@rjbs rjbs merged commit 07267d6 into rjbs:master Mar 25, 2016
@rjbs
Copy link
Owner

rjbs commented Mar 25, 2016

Thanks! Applied and released.

dolmen added a commit to dolmen/p5-Moose that referenced this pull request Mar 30, 2016
Because it has a major optimisations:
rjbs/Data-OptList#1
dolmen added a commit to dolmen/p5-Dist-Zilla that referenced this pull request Apr 21, 2016
rjbs pushed a commit to rjbs/Dist-Zilla that referenced this pull request Apr 21, 2016
dolmen added a commit to dolmen/p5-Moose that referenced this pull request Apr 22, 2016
karenetheridge pushed a commit to moose/Moose that referenced this pull request Apr 30, 2016
mephinet pushed a commit to mephinet/Moose that referenced this pull request Nov 24, 2017
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

3 participants