1371593 - Rebinding a bound function multiple times calls AtomizeString each time

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[André Bargull [:anba]]

This µ-benchmark completes in 110ms for me:

    var bf = function f(){};
    var t = Date.now();
    for (var i = 0; i < 1000000; ++i) {
        bf.bind();
    }
    print(Date.now() - t);

Whereas this one requires 180ms:

    var bf = function f(){}.bind();
    var t = Date.now();
    for (var i = 0; i < 1000000; ++i) {
        bf.bind();
    }
    print(Date.now() - t);


Avoiding these repeated string atomization calls (http://searchfox.org/mozilla-central/rev/a798ee4fc323f9387b7576dbed177859d29d09b7/js/src/jsfun.cpp#1399-1405) would make the second benchmark as fast as the first one.

This issue is visible in Speedometer/react and also in local code (bug 1371287).

Comment 1

[Till Schneidereit [:till]]

I wonder if we can perhaps avoid the atomization entirely in the common case. Usually, the function name isn't every queried, so atomizing the "bound foo" string is useless overhead. Would it work to instead just set the function's atom to the source function's name and set a flag on the function to do the atomization in the resolve hook?

Hmm, I guess that wouldn't really help this particular case, at least if we don't want to make the resolve hook check source functions recursively. Perhaps we should actually do that?

Comment 2

[André Bargull [:anba]]

(In reply to André Bargull from comment #0)
> This issue is visible in Speedometer/react [...].

Specifically these lines call bind() on already bound function objects: https://github.com/WebKit/webkit/blob/21bb594184b731a990f1c0389ca61352ae9089af/PerformanceTests/Speedometer/resources/todomvc/architecture-examples/react/js/app.jsx#L369-L373

Comment 3

[André Bargull [:anba]]

(In reply to Till Schneidereit [:till] from comment #1)
> I wonder if we can perhaps avoid the atomization entirely in the common
> case. Usually, the function name isn't every queried, so atomizing the
> "bound foo" string is useless overhead. Would it work to instead just set
> the function's atom to the source function's name and set a flag on the
> function to do the atomization in the resolve hook?

If it's possible to get space resp. reuse an otherwise unused flag in JSFunction::flags_ to store this bit, we could easily improve this test case.

> Hmm, I guess that wouldn't really help this particular case, at least if we
> don't want to make the resolve hook check source functions recursively.
> Perhaps we should actually do that?

There's one issue when naively traversing the chain of bound functions. We somehow need to detect if one of the bound functions has a redefined .name property:

  function foo(){}
  var bf1 = foo.bind();  // JSFunction::atom_ = "foo", .name = "bound foo"
  Object.defineProperty(bf1, "name", {value: "bar"});  // JSFunction::atom_ = "foo", .name = "bar"
  var bf2 = bf1.bind();  // JSFunction::atom_ = "bar", .name = "bound bar"

In this case we can't simply traverse the chain of bound functions to determine the correct .name for |bf2|, because |bf1| has a redefined .name property when |bf1.bind()| was called.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to André Bargull from comment #3)
> If it's possible to get space resp. reuse an otherwise unused flag in
> JSFunction::flags_ to store this bit, we could easily improve this test case.

Maybe reuse the HAS_GUESSED_ATOM flag? That should never be set on bound functions AFAIK, so hasGuessedAtom() could just return false for bound functions. We could rename HAS_GUESSED_ATOM to make this clearer.

Comment 5

[Till Schneidereit [:till]]

(In reply to André Bargull from comment #3)
> There's one issue when naively traversing the chain of bound functions. We
> somehow need to detect if one of the bound functions has a redefined .name
> property:
> 
>   function foo(){}
>   var bf1 = foo.bind();  // JSFunction::atom_ = "foo", .name = "bound foo"
>   Object.defineProperty(bf1, "name", {value: "bar"});  // JSFunction::atom_
> = "foo", .name = "bar"
>   var bf2 = bf1.bind();  // JSFunction::atom_ = "bar", .name = "bound bar"
> 
> In this case we can't simply traverse the chain of bound functions to
> determine the correct .name for |bf2|, because |bf1| has a redefined .name
> property when |bf1.bind()| was called.

We can check for that by testing if .name was resolved, right? If so, we use that as the base case for building the full name, prefixing with "bound " as needed.

(In reply to Jan de Mooij [:jandem] from comment #4)
> (In reply to André Bargull from comment #3)
> > If it's possible to get space resp. reuse an otherwise unused flag in
> > JSFunction::flags_ to store this bit, we could easily improve this test case.
> 
> Maybe reuse the HAS_GUESSED_ATOM flag? That should never be set on bound
> functions AFAIK, so hasGuessedAtom() could just return false for bound
> functions. We could rename HAS_GUESSED_ATOM to make this clearer.

Agreed, that seems like it should work.

Comment 6

[André Bargull [:anba]]

Attachment: bug1371593.patch

This passes jstest and jit-tests, but now I wonder if there are any complications in FrameIter::matchCallee (http://searchfox.org/mozilla-central/rev/a798ee4fc323f9387b7576dbed177859d29d09b7/js/src/vm/Stack.cpp#1103-1110), because HAS_GUESSED_ATOM is part of STABLE_ACROSS_CLONES ?

Comment 7

[Till Schneidereit [:till]]

Comment on attachment 8876119 [details] [diff] [review]
bug1371593.patch

Review of attachment 8876119 [details] [diff] [review]:
-----------------------------------------------------------------

Looks reasonable to me, but I can't really comment on the matchCallee concern, so I'll leave that to jandem.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8876119 [details] [diff] [review]
bug1371593.patch

Review of attachment 8876119 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch regarding STABLE_ACROSS_CLONES. STABLE_ACROSS_CLONES is only used by matchCallee, so I think it's fine to just remove the HAS_GUESSED_ATOM flag from it. It's just an heuristic to determine whether functions are definitely unrelated.

Comment 9

[André Bargull [:anba]]

Attachment: bug1371593.patch

Updated patch.

Changes compared to the previous version:
- Removed HAS_GUESSED_ATOM from STABLE_ACROSS_CLONES
- Added extra assertions to test setGuessedAtom/clearGuessedAtom is never called with bound functions

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8876581 [details] [diff] [review]
bug1371593.patch

Review of attachment 8876581 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 11

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cab9c7d94d21f175ec0b66dcab69cc9aea49b6f3

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/20aba2431d94
Avoid repeated string atomizations when retrieving the unresolved name of a bound function. r=jandem

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/20aba2431d94