constants system rewrite (in two parts) #1653

elia · 2017-04-03T22:54:17Z

~~🙅‍♂️ DO NOT MERGE YET~~

a complete rewrite of the constants system to actually mimic CRuby behavior

fixes [edge-case] constant lookup diverge from MRI #548
fixes Failing module specs with some rand seeds #1607
related: autoload lookup differs from MRI #1581 Integrate with Opal master asciidoctor/asciidoctor.js#280 Include nested module with same name #1577

To do before merging:

remove debug code and debug/stale comments
update API documentation
update guides (if needed)

Part 1: Make it correct ✅

This is more or less done:

the lookup correctly treats relative and qualified constant names
nesting is passed around for each scope in the form of an array with the nearest nesting first
Module.nesting and Module.constants correctly work
BasicObject behaves properly (see [edge-case] constant lookup diverge from MRI #548)

Part 2: Make it fast ⏳

This is still to be done, some considerations:

a constant lookup cache should probably be created
consider re-using Adam's very smart idea of a prototype chain for constants (can work for both inherited and nesting lookups)
remember that any cache must be properly invalidated or updated
consider using a property on $nesting
consider using shorter names
consider restoring some form of Opal.get

iliabylich

Just a 👏 . Great work, I would say partially broken constant lookup was a number 1 issue. You are a hero 😄 . Are there any intermediate benchmark results? And did you try to run the rubyspec test suite with those failing random seeds?

iliabylich · 2017-04-03T23:48:04Z

lib/opal/nodes/call.rb

+      add_special :nesting do |compile_default|
+        recv, method, *args = children
+        push_nesting = push_nesting?(children)
+        push '(Opal.Module.$$nesting = $nesting, ' if push_nesting


Is it necessary to populate Opal.Module.$$nesting and call the default behavior? It's a special case for CallNode anyway.

I thought that overriding Module.nesting should be supported and I'd be very curious to see an app that has this as a bottleneck 😄 – Also could be a different Module than ::Module.

iliabylich · 2017-04-03T23:51:03Z

lib/opal/nodes/call.rb

+        recv = children.first
+
+        children.size == 2 && (           # only receiver and method
+          recv.nil? || (                  # and no receiver


I think it could be replaced with recv.nil? || recv == s(:const, nil, :Module). Is it written this way to cover both Module and ::Module (with nil and s(:cbase) as constant scopes)?

Yes, I think I meant to cover both nil and s(:cbase). This part could probably use a test.

iliabylich · 2017-04-03T23:53:11Z

opal/corelib/basic_object.rb

  def instance_eval(*args, &block)
    if block.nil? && `!!Opal.compile`
-      Kernel.raise ArgumentError, "wrong number of arguments (0 for 1..3)" unless (1..3).cover? args.size
+      ::Kernel.raise ::ArgumentError, "wrong number of arguments (0 for 1..3)" unless (1..3).cover? args.size


Is it required to put :: everywhere now to avoid calling local constants? (Like potentially generated on the fly BasicObject::Kernel)

no, it's because from inside BasicObject you can't access top level constants… except by making the lookup qualified with the leading double-dots and ignore Module.nesting.

See also the changes in native.rb

elia · 2017-04-04T17:17:18Z

Are there any intermediate benchmark results?

No, still need to start on that side

And did you try to run the rubyspec test suite with those failing random seeds?

yes, and there was no error 😸, even before adding all those specs

iliabylich

A few small notes, also there's some commented out code in runtime.js that most probably could be removed. 👍 from me even without benchmarks. working correctly is more important than being fast 😄

iliabylich · 2017-04-05T00:18:28Z

opal/corelib/runtime.js

-  Opal.constants.push("Object");
-  Opal.constants.push("Module");
-  Opal.constants.push("Class");
+  // BasicObject can reach itself, avoid const_set to skup the


iliabylich · 2017-04-05T00:21:46Z

stdlib/json.rb


            if (!options.parse && (klass = #{`hash`[JSON.create_id]}) != nil) {
-              klass = Opal.get(klass);
+              klass = Opal.const_get_qualified('::', klass);


Is it possible to rewrite it using const_get from Ruby? If it's a global constant probably Object.const_get may work here. For me "::" looks like a private api from runtime.js that shouldn't be used explicitly in corelib/stdlib

@iliabylich I think the important point here is that we should define the public runtime API before 1.0

meh · 2017-04-06T23:39:25Z

I'm going to review it this weekend, I did read it already when you pushed the first stuff, all I could think of is the diff is unreadable.

EDIT: wait, it was unreadable, or I'm thinking of something else, or whatever, what are you still doing awake?

elia · 2017-04-07T21:07:33Z

EDIT: wait, it was unreadable, or I'm thinking of something else, or whatever, what are you still doing awake?

@meh I did a bunch of side stuff that was initially in the PR but then I separated and pushed to master, that's probably why now it's more readable 📖

Anyway I'd like to hear some thoughts on this from you before proceeding in trying to optimize for performance, so looking forward to your review ✏️ 👓

meh

Looks good other than those two nits.

meh · 2017-04-09T15:44:13Z

lib/opal/nodes/top.rb


          if compiler.eval?
-            add_temp '$scope = (self.$$scope || self.$$class.$$scope)'
+            add_temp '$nesting = self.$$is_a_module ? [self] : [self.$$class]'


Shouldn't this be $$is_module instead of $$is_a_module?

@meh there's $$is_module and $$is_class that XOR on modules and classes, but there's also $$is_a_module that is true for both and replaces $$is_module || $$is_class checks.

That said I agree that a better name is desirable.

Maybe $$is_namespace? Since that's what the check is looking for.

@meh wrt the $$is_a_module prop I'll open a separate issue/PR

update: #1654

meh · 2017-04-09T16:14:54Z

opal/corelib/module.rb

+      } else {
+        #{raise NameError, "constant #{self}::#{name} not defined"}
+      }
    }


All of this would probably look better if you removed the else if { } else { } since you're returning from each of them.

elia · 2017-04-10T00:02:54Z

current status of perf vs. master:

opal:master ⤑ be bin/opal -g benchmark-ips -ropal/platform -rbenchmark/ips -e 'module A; module B; Benchmark.ips do |x|; x.report("Kernel") { Kernel }; x.report("::Kernel"){ ::Kernel };x.compare!;end; end; end'
Warming up --------------------------------------
              Kernel    28.156k i/100ms
            ::Kernel    29.693k i/100ms
Calculating -------------------------------------
              Kernel      2.815M (± 5.8%) i/s -     14.022M in   5.002232s
            ::Kernel      3.879M (± 6.0%) i/s -     19.300M in   4.997887s

Comparison:
            ::Kernel:  3879237.1 i/s
              Kernel:  2814554.1 i/s - 1.38x  slower

opal:elia/constants ⤑ be bin/opal -g benchmark-ips -ropal/platform -rbenchmark/ips -e 'module A; module B; Benchmark.ips do |x|; x.report("Kernel") { Kernel }; x.report("::Kernel"){ ::Kernel };x.compare!;end; end; end'
Warming up --------------------------------------
              Kernel     8.394k i/100ms
            ::Kernel    10.546k i/100ms
Calculating -------------------------------------
              Kernel    606.318k (± 5.9%) i/s -      3.022M in   5.004157s
            ::Kernel      6.897M (± 7.9%) i/s -     33.937M in   4.962153s

Comparison:
            ::Kernel:  6896933.5 i/s
              Kernel:   606317.8 i/s - 11.38x  slower

elia · 2017-04-11T21:50:28Z

After adding the cache perf go back more than acceptable:

bundle exec opal -ropal/platform -gbenchmark-ips -rbenchmark/ips -A /Users/elia/Code/opal/benchmark-ips/bm_constants_lookup.rb
Warming up --------------------------------------
              Kernel    99.137k i/100ms
            ::Kernel    85.107k i/100ms
Calculating -------------------------------------
              Kernel      7.586M (± 6.6%) i/s -     37.771M in   5.004006s
            ::Kernel      4.149M (± 7.2%) i/s -     20.681M in   5.015473s

Comparison:
              Kernel:  7585546.3 i/s
            ::Kernel:  4148983.1 i/s - 1.83x  slower

bm_constants_lookup.rb

module A
  module B
    module C
      Benchmark.ips do |x|
        x.report("Kernel") { Kernel }
        x.report("::Kernel"){ ::Kernel }
        x.compare!
      end
    end
  end
end

elia · 2017-04-11T22:13:57Z

Checking performance at each commit shows an overall speed improvement from 50% to almost 80% (details in this gist). 🏎

meh · 2017-04-11T23:23:00Z

Isn't that going to slow down startup considerably, or am I reading it wrong?

I can already see @jgaskins yelling at us.

jgaskins · 2017-04-12T04:20:31Z

Some things might be a little slower, but I'd be interested in seeing what this looks like in a real-world app. Because some constants are referenced over and over just during bootstrapping of an app, the extra cost might be amortized. I'll run a benchmark tomorrow and see what I can find out.

elia · 2017-04-12T06:11:31Z

@meh @jgaskins tbh I think the opposite to be true, the caching price is paid only the first time a const is referenced and only if it is referenced, previously it was upfront for any constant, even if never used. Also the cache is invalidated globally for any const change but it's done locally and only for referenced constants. Unless an app is going to keep modifying constants at runtime it will be much better. 😄

jgaskins · 2017-04-13T04:49:47Z

Okay, quick analysis before bedtime (that's a lie, I actually should've been in bed an hour ago).

This is how I usually run load-time benchmarks:

<div>
  <span>Load time: </span>
  <span id="benchmark-result"></span>
</div>
<script>var started_at = performance.now()</script>
<script src="app.js"></script>
<script>document.getElementById('benchmark-result').textContent = (performance.now() - started_at).toString()</script>

Then I load the page a bunch of times to get a feel for where the number converges. The app is compiled/minified/gzipped just as you would in production to remove the overhead of multiple requests.

The app I chose to test this with was 1543 LoC (not counting blank lines and comments). There are 139 top-level constants. It uses Clearwater, GrandCentral, and Opal::Pusher, which have a few nested constants, but not a whole lot. Maybe 30 across all of them. The benchmark values below only include load time without rendering (Clearwater renders with requestAnimationFrame, so the finish-time check occurs while the render is still in the queue). Rendering does access a bunch of constants, as well, and it's important to capture as much as we can, but I think that introduces a bunch of other variables Opal can't control for. I just wanted to have a bunch of constants declared at load time.

On Chrome, the numbers converged around 148ms for this PR vs 157ms on the master branch (5.7% faster).

On Safari, it reduced it to 72ms from 78ms (7.7% faster).

On Firefox, this was a little slower than master, but I have a feeling it takes Firefox longer to warm up its JIT because the numbers were 242ms vs 253ms. At that point, a few milliseconds in either direction may not mean much. I also have no idea what's causing this slowdown.

This PR also had a lot less fluctuation in the load-time benchmarks than the master branch. The numbers still fluctuated and there were still outliers, but it really felt like the range was smaller. I don't have hard data to back that up, though, because I wasn't recording individual times.

It seems like a load-time performance win, tbh. Slower in Firefox, but the vast majority of mobile users (where load-time performance matters most) are on Chrome or Safari. I'll try to test a bit more on mobile devices this weekend if I get a chance.

@ConradIrwin

Still needs some fixes for proper autoloading checks and to go back to acceptable speed. Special thanks to: - The Rails autoloading guide (http://guides.rubyonrails.org/v5.0/autoloading_and_reloading_constants.html) - @ConradIrwin's 2012 post on “Everything you ever wanted to know about constant lookup in Ruby” (http://cirw.in/blog/constant-lookup.html)

- It will expire every time a constant is defined, removed or set. - For relative lookups the cache will be held on the current nesting. - For qualified lookups the cache will be held on the current cref. - Only the referenced constants will be cached. There's a slight risk of memory leak if constants are set dynamically and their unreferenced, but seems a good overall compromise that can only be resolved by a WeakRef JS implementation.

This follows the fix done in opal/opal#1653 (constants system rewrite)

elia added corelib performance runtime labels Apr 3, 2017

elia added this to the v0.11 milestone Apr 3, 2017

elia self-assigned this Apr 3, 2017

elia force-pushed the elia/constants branch from 670405e to 999640d Compare April 3, 2017 23:06

iliabylich reviewed Apr 4, 2017

View reviewed changes

elia force-pushed the elia/constants branch from 999640d to 1248d79 Compare April 4, 2017 17:23

iliabylich approved these changes Apr 5, 2017

View reviewed changes

elia requested a review from meh April 6, 2017 23:12

meh requested changes Apr 9, 2017

View reviewed changes

elia mentioned this pull request Apr 10, 2017

Changing/switching the name of the $$is_a_module property #1654

Closed

elia force-pushed the elia/constants branch 3 times, most recently from c002787 to e77b07f Compare April 11, 2017 21:35

Establish a constant lookup IPS benchmark

447e1b4

elia force-pushed the elia/constants branch from e77b07f to effc86f Compare April 11, 2017 22:01

meh approved these changes Apr 13, 2017

View reviewed changes

elia force-pushed the elia/constants branch from effc86f to 2b86cf0 Compare April 13, 2017 22:50

elia force-pushed the elia/constants branch from 2b86cf0 to 1676712 Compare April 13, 2017 22:53

elia merged commit 2574b79 into master Apr 13, 2017

elia deleted the elia/constants branch April 13, 2017 23:33

elia mentioned this pull request Apr 13, 2017

Integrate with Opal master asciidoctor/asciidoctor.js#280

Merged

jgaskins mentioned this pull request Apr 14, 2017

Failing spec for begin/end with early return that passes on MRI #1656

Merged

bwl21 added a commit to bwl21/zupfnoter that referenced this pull request Dec 19, 2017

improve scoping of constants

d2f88a1

This follows the fix done in opal/opal#1653 (constants system rewrite)

Uh oh!

constants system rewrite (in two parts) #1653

constants system rewrite (in two parts) #1653

Conversation

elia commented Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Part 1: Make it correct ✅

Part 2: Make it fast ⏳

Uh oh!

iliabylich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elia commented Apr 4, 2017

Uh oh!

iliabylich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meh commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elia commented Apr 7, 2017

Uh oh!

meh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elia Apr 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elia commented Apr 10, 2017

Uh oh!

elia commented Apr 11, 2017

Uh oh!

elia commented Apr 11, 2017

Uh oh!

meh commented Apr 11, 2017

Uh oh!

jgaskins commented Apr 12, 2017

Uh oh!

elia commented Apr 12, 2017

Uh oh!

jgaskins commented Apr 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

elia commented Apr 3, 2017 •

edited

Loading

meh commented Apr 6, 2017 •

edited

Loading

elia Apr 10, 2017 •

edited

Loading