Module#prepend #1826

iliabylich · 2018-05-14T23:17:00Z

Not ready yet.

Includes a massive rewrite of the runtime.js

The most important: Ancestors chain is represented via prototypes (for subclassing and modules inclusion).
Every Opal class has BasicObject on the bottom of the prototype chain. It makes stubs logic much, much easier. (And I think in general the logic is easier now).
Removed $$parent, $$included_modules, $$ancestors, $$children, $$methods.
There's no method donation anymore. Yes, method definition = prototype[method_name] = body + updating a list of iclasses + hooks.
All Objects can (and actually are) allocated via new Opal.ClassName.
Inheritance from String is simplified (there are only 5 or 6 failing specs, not sure if it's worth to revert it).

RubySpec suite "works for me" (for both node and chrome), going to check other tasks tomorrow.

Also this PR is called Module#prepend, so I'm also going to add Opal.prepend_features, it looks straightforward now.

Also I've just realized that we could use obj instanceof Opal.SomeClass for classes (for modules there's an iclass in the prototype chain, so it won't work). That's funny.

…n). module/ancestors_spec is green.

…(&block).

… obj.clone. language specs are green.

…ased methods.

…een.

iliabylich · 2018-05-15T18:41:01Z

@Mogztter Could you check please if it works with the Asciidoctor?

iliabylich · 2018-05-15T18:41:29Z

Basically ready for reviewing.

elia · 2018-05-15T22:46:05Z

@iliabylich super great stuff man!
I can't wait to read the diff (yes, I still have to, and yes, I had to wait until now 😅)

I didn't know about the way prepend was implemented in MRI, that's clever! Basically any Module instance (including classes) will defn on the list of protos it maintains, where:

for a regular class it's just [self.prototype]
for any module is the list of its iclasses
for a class with prepend it's the one iclass (having prepend_feature moving all methods from the prototype to the iclass)

is that correct?

now proceeding to the review! 😄

iliabylich · 2018-05-15T23:03:03Z

is that correct?

Yes!

elia · 2018-05-15T23:42:09Z

opal/corelib/kernel.rb


        for (i = 0, length = names.length; i < length; i++) {
          name = names[i];
-          if (name.charAt(0) === '$') {
-            self_singleton_class_proto[name] = other_singleton_class_proto[name];
+          if (name.charAt(0) === '$' && name !== '$$id') {


Maybe it's worth exposing Opal.is_method or using the same check?

elia · 2018-05-16T00:17:01Z

opal/corelib/kernel.rb

+        self_singleton_class.$$const = Object.assign({}, other_singleton_class.$$const);
+        Object.setPrototypeOf(
+          self_singleton_class.prototype,
+          Object.getPrototypeOf(other_singleton_class.prototype)


Have you considered https://stackoverflow.com/a/23809148 ?

seems to have better performance at startup, but degrades in other cases, probably it's worth it anyway, but I'd like to be sure firefox does not become unbearably slow with proto changes

perf of rake mspec_ruby_nodejs

# prepend 11031 examples, 0 failures (time taken: 13.869999885559082) 56.68 real 55.76 user 2.29 sys # master 11009 examples, 0 failures (time taken: 10.92799997329712) 54.29 real 52.99 user 2.37 sys

Yeah, that's because there's too much dynamic stuff in the RubySpec suite. It create anonymous classes, extends them in runtime, I guess that's the reason. For a static system of modules it seems to be faster.

👍🏼 I need to test this on FF/mobile/… with more realistic code, just to be sure, anyway using prototypes makes everything 💯 times cleaner ✨

elia · 2018-05-16T00:23:29Z

opal/corelib/runtime.js

    }

-    // Class doesnt exist, create a new one with given superclass...
+    throw new Error('Broken prototype chain');
+  }


superclass is fixed and should be easy to cache, probably a good one after this one is done ✅

Not sure what do you mean here

I mean that we should consider storing a ref to the superclass right inside the class, since it can't change in ruby

elia · 2018-05-16T00:25:42Z

opal/corelib/runtime.js

+    // If scope is an object, use its class
+    if (!scope.$$is_class && !scope.$$is_module) {
+      scope = scope.$$class;
+    }


This can be moved inside the else of if (scope == null)

elia · 2018-05-17T09:11:28Z

opal/corelib/runtime.js

@@ -1341,7 +1216,8 @@
  //
  // @default [Prototype List] BasicObject_alloc.prototype
  //
-  Opal.stub_subscribers = [BasicObject_alloc.prototype];
+  // FIXME


Can you add some notes on what's to fix?

Yep, we don't need stub_subscribers anymore (because all bridged classes are inherited from the BasicObject). The fix is to remove it 😄

…ion.

iliabylich · 2018-05-17T14:03:38Z

I'm getting the following results

RubySpec suite

1007 static classes, 589 static modules, 299 includes, 59 extends.
332 Class.new invocations, 134 Module.new

Chrome:

master: 11009 examples, 41 failures (time taken: 13.717999935150146)
patch: 11031 examples, 41 failures (time taken: 16.236999988555908)

FF:

master: 11009 examples, 41 failures (time taken: 24.863999843597412)
patch: 11031 examples, 41 failures (time taken: 29.003000020980835)

Safari:

master: 11009 examples, 43 failures (time taken: 19.655999898910522)
patch: 11031 examples, 43 failures (time taken: 21.611000061035156)

Boot time

78 static classes, 19 static modules, 11 includes, 2 extends.
0 Class.new invocations, 0 Module.new

Measured using

window.onload = function () {
  var loadTime = window.performance.timing.domContentLoadedEventEnd-window.performance.timing.navigationStart;
  console.log('Full load time is '+ loadTime / 1000);
}

Chrome

master: Page load time is 0.167
patch: Full load time is 0.105

FF:

master: Full load time is 0.139
patch: Full load time is 0.104

Safari:

master: Full load time is 0.343
patch: Full load time is 0.263

iliabylich · 2018-05-17T14:17:02Z

More benchmarks on V8:

function benchmark() {
  echo $1 > test.rb
  gco master
  opal -c test.rb > test.js
  echo "Master:"
  repeat 5 time node test.js
  gco module-prepend
  echo "Patch:"
  opal -c test.rb > test.js
  repeat 5 time node test.js
}

Class.new:

$ benchmark "1000.times { Class.new }"
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
Master:
  0.28s user 0.04s system 99% cpu 0.313 total
  0.27s user 0.04s system 100% cpu 0.310 total
  0.28s user 0.04s system 100% cpu 0.314 total
  0.27s user 0.04s system 100% cpu 0.305 total
  0.27s user 0.04s system 99% cpu 0.309 total
Switched to branch 'module-prepend'
Your branch is up to date with 'origin/module-prepend'.
Patch:
  0.18s user 0.03s system 99% cpu 0.217 total
  0.19s user 0.03s system 97% cpu 0.226 total
  0.19s user 0.03s system 97% cpu 0.226 total
  0.19s user 0.03s system 96% cpu 0.232 total
  0.19s user 0.03s system 96% cpu 0.230 total

Module.new:

$ benchmark "1000.times { Module.new }"
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
Master:
  0.26s user 0.04s system 99% cpu 0.295 total
  0.26s user 0.04s system 99% cpu 0.298 total
  0.27s user 0.04s system 98% cpu 0.310 total
  0.27s user 0.04s system 99% cpu 0.312 total
  0.26s user 0.04s system 99% cpu 0.295 total
Switched to branch 'module-prepend'
Your branch is up to date with 'origin/module-prepend'.
Patch:
  0.17s user 0.03s system 98% cpu 0.198 total
  0.18s user 0.03s system 97% cpu 0.214 total
  0.20s user 0.03s system 97% cpu 0.236 total
  0.18s user 0.03s system 97% cpu 0.213 total
  0.18s user 0.03s system 96% cpu 0.224 total

Module#include:

$ benchmark "K = Class.new; 1000.times { K.include(Module.new) }"
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
Master:
  0.45s user 0.04s system 100% cpu 0.491 total
  0.46s user 0.04s system 100% cpu 0.499 total
  0.46s user 0.04s system 100% cpu 0.499 total
  0.46s user 0.04s system 101% cpu 0.487 total
  0.47s user 0.04s system 100% cpu 0.503 total
Switched to branch 'module-prepend'
Your branch is up to date with 'origin/module-prepend'.
Patch:
  2.67s user 0.04s system 99% cpu 2.710 total
  3.55s user 0.04s system 99% cpu 3.605 total
  5.18s user 0.05s system 99% cpu 5.239 total
  2.42s user 0.04s system 99% cpu 2.483 total
  2.42s user 0.04s system 99% cpu 2.462 total

singleton_class:

$ benchmark "1000.times { Object.new.singleton_class }"
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
Master:
  0.26s user 0.04s system 97% cpu 0.302 total
  0.26s user 0.03s system 99% cpu 0.287 total
  0.25s user 0.03s system 100% cpu 0.286 total
  0.26s user 0.03s system 99% cpu 0.295 total
  0.26s user 0.03s system 100% cpu 0.291 total
Switched to branch 'module-prepend'
Your branch is up to date with 'origin/module-prepend'.
Patch:
  0.19s user 0.03s system 99% cpu 0.225 total
  0.20s user 0.03s system 97% cpu 0.238 total
  0.20s user 0.03s system 97% cpu 0.235 total
  0.20s user 0.03s system 97% cpu 0.232 total
  0.20s user 0.03s system 97% cpu 0.236 total

So Module#include seems to be a bottleneck, it's 5-7 times slower. btw, Module#prepend has the same performance (because it does pretty much the same thing)

iliabylich · 2018-05-17T14:39:22Z

Actually it's not that bad when the ancestors chain is small:

$ benchmark "1000.times { Class.new { include Module.new }}"
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
Master:
  0.36s user 0.04s system 103% cpu 0.390 total
  0.37s user 0.04s system 103% cpu 0.392 total
  0.37s user 0.04s system 103% cpu 0.400 total
  0.36s user 0.04s system 103% cpu 0.389 total
  0.36s user 0.04s system 103% cpu 0.387 total
Switched to branch 'module-prepend'
Your branch is up to date with 'origin/module-prepend'.
Patch:
  0.26s user 0.03s system 105% cpu 0.283 total
  0.27s user 0.04s system 104% cpu 0.293 total
  0.26s user 0.04s system 104% cpu 0.288 total
  0.26s user 0.04s system 103% cpu 0.285 total
  0.25s user 0.04s system 104% cpu 0.282 total

Also I've mirco-benchmarked Opal.append_features for this specific case:

3937.128662109375 μs - getting a list of module's ancestors.
5101.341552734375 μs - build iclasses.
6154.453857421875 μs - building a chain of iclasses (i.e. chaining prototypes)
568.290283203125 μs finding a place in existing prototypes chain to insert a new one
2381.034912109375 μs - updating module's prototype (i.e. inserting a chain if iclasses)

ggrossetie · 2018-05-17T15:10:00Z

@iliabylich I'm wondering why FF is so slow compared to Chrome to run the RubySpec suite ? What version are you using ?

Also the performance of module#include is intriguing... basically it's faster when we include a module only once into a class but it's 5-7 slower when we include 1000 modules to the same class ?
I didn't have time to read the code yet but I guess we are doing checks on the class that's why include is getting slower and slower ?

iliabylich · 2018-05-17T16:08:54Z

What version are you using ?

60.0.1

it's faster when we include a module only once into a class but it's 5-7 slower when we include 1000 modules to the same class ?

exactly.

we are doing checks on the class that's why include is getting slower and slower ?

From what I understand for a case when we have a huge ancestors chain the time is mostly taken by the part that rebuilds a prototype chain. Maybe JS engines do some recalculations when it happens, I don't know. But anyway, that's a rare case, so I think it's fine.

ggrossetie · 2018-05-17T20:45:22Z

True. I will run your branch against the Asciidoctor.js benchmark to see if there's a performance gain :)
But as far as I know we are not using prepend anymore.

elia · 2018-05-17T21:45:06Z

From what I understand for a case when we have a huge ancestors chain the time is mostly taken by the part that rebuilds a prototype chain. Maybe JS engines do some recalculations when it happens, I don't know. But anyway, that's a rare case, so I think it's fine.

https://jsperf.com/long-proto-chain/1 seems confirmed, the difference with building a proto chain with Object.create is astounding, yet I agree that's fine as it is

…ry class/module to speedup ancestors calculation.

…prepend.

iliabylich · 2018-05-24T20:37:53Z

@elia Did you have a chance to check it? The fix for caching $$ancestors is here, can we merge it?

elia · 2018-05-24T21:52:37Z

@iliabylich merge at will

iliabylich added 30 commits May 10, 2018 18:10

Simplified super() using $$ancestors.

838ea99

Added basic implementation of Module#prepend.

2d8b59e

WIP.

12efd6a

WIP: almost works.

5f537cf

WIP: p 42 works.

51e01df

WIP: fixed super.

eab916c

WIP: opal -ropal-parser -e "p eval('42')" works as well.

9726276

WIP: minor cleanup. test suite is runnable (but mostly broken).

d64c8b6

WIP: properly include nested modules (introduced create_iclasses_chai…

4457109

…n). module/ancestors_spec is green.

WIP: Removed obsolete runtime functions.

86fb636

WIP: Fixed Module#instance_methods, restored Object.prototype.$to_s

f8b0630

WIP: removed debug output.

c843b05

WIP: restored stubs generation. Fixed existing module reinitialization.

2b6f05a

WIP: fixed Class.new.class to be Class; Evaluate a block on Class.new…

b6ef448

…(&block).

WIP: removed unused method.

9297bc0

WIP: simplified inheritance from String.

07d0b04

WIP: restored back module_function and metaclass constants copying on…

e47f4c9

… obj.clone. language specs are green.

WIP: Fixed super in the aliased method.

a2539f5

WIP: reverted back detecting cyclic include.

7d90d9a

WIP: reverted back calling method_added/singleton_method_added.

5f9c489

WIP: Fixed inheritance from module.

dddb7d0

WIP: re-introduced method.$$owner to properly implement super for ali…

fe4efb1

…ased methods.

WIP: Added Opal.included_modules.

f2858e1

WIP: Include missing modules recursively on 2nd include.

61a777a

WIP: bump constants cache after module inclusion. module_specs are gr…

c3c8f9c

…een.

WIP: run .inherited for dynamically generated classes.

27e97fc

WIP: removed dead code.

ca4a6ea

WIP: fixed bridging, 42 is a Number, "" is a String.

8c77ff4

WIP: do not try to define a singleton method on primitives.

3d3175b

WIP: convert to string subclasses in String methods.

e85a8ee

iliabylich requested review from elia, meh and ggrossetie May 15, 2018 18:39

elia reviewed May 17, 2018

View reviewed changes

iliabylich added 4 commits May 17, 2018 15:42

CR: Use $$super instead of superclassOf() function.

600b7c3

CR: expose is_method as Opal.is_method.

d6a4845

CR: simplified logic that determines a scope for class/module definit…

8a20fab

…ion.

CR: removed unused code.

c195b47

elia approved these changes May 17, 2018

View reviewed changes

iliabylich added 4 commits May 18, 2018 21:32

perf: added $$own_included_modules and $$own_prepended_modules to eve…

6687af9

…ry class/module to speedup ancestors calculation.

perf: Optimize even more by setting $$own_ancestors on every include/…

e6cf952

…prepend.

perf: Use Opal.const_cache_version to cache module.$$ancestors.

6646a07

__proto__ -> getPrototypeOf.

ff1d635

iliabylich merged commit 3d46dd4 into opal:master May 25, 2018

iliabylich deleted the module-prepend branch May 25, 2018 08:14

elia mentioned this pull request May 27, 2018

inherited issues #1196

Closed

iliabylich mentioned this pull request Jun 1, 2018

Module#prepend not implemented #796

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Module#prepend #1826

Module#prepend #1826

iliabylich commented May 14, 2018

iliabylich commented May 15, 2018

iliabylich commented May 15, 2018

elia commented May 15, 2018

iliabylich commented May 15, 2018

elia May 15, 2018

elia May 16, 2018

iliabylich May 17, 2018

elia May 17, 2018

elia May 16, 2018

iliabylich May 17, 2018

elia May 17, 2018

elia May 16, 2018

elia May 17, 2018

iliabylich May 17, 2018

iliabylich commented May 17, 2018

iliabylich commented May 17, 2018

iliabylich commented May 17, 2018 •

edited

Loading

ggrossetie commented May 17, 2018

iliabylich commented May 17, 2018

ggrossetie commented May 17, 2018

elia commented May 17, 2018

iliabylich commented May 24, 2018

elia commented May 24, 2018

Module#prepend #1826

Module#prepend #1826

Conversation

iliabylich commented May 14, 2018

iliabylich commented May 15, 2018

iliabylich commented May 15, 2018

elia commented May 15, 2018

iliabylich commented May 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iliabylich commented May 17, 2018

RubySpec suite

Boot time

iliabylich commented May 17, 2018

iliabylich commented May 17, 2018 • edited Loading

ggrossetie commented May 17, 2018

iliabylich commented May 17, 2018

ggrossetie commented May 17, 2018

elia commented May 17, 2018

iliabylich commented May 24, 2018

elia commented May 24, 2018

iliabylich commented May 17, 2018 •

edited

Loading