Rebinding a bound function multiple times calls AtomizeString each time

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: anba, Assigned: anba)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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).
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?
(Assignee)

Comment 2

2 years ago
(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
(Assignee)

Comment 3

2 years ago
(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.
(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.
(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.
(Assignee)

Comment 6

2 years ago
Posted patch bug1371593.patch (obsolete) — Splinter Review
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 ?
Attachment #8876119 - Flags: feedback?(till)
Attachment #8876119 - Flags: feedback?(jdemooij)
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.
Attachment #8876119 - Flags: feedback?(till) → feedback+
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.
Attachment #8876119 - Flags: feedback?(jdemooij) → feedback+
(Assignee)

Comment 9

2 years ago
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
Assignee: nobody → andrebargull
Attachment #8876119 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8876581 - Flags: review?(jdemooij)
Comment on attachment 8876581 [details] [diff] [review]
bug1371593.patch

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

Thanks!
Attachment #8876581 - Flags: review?(jdemooij) → review+

Comment 12

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/20aba2431d94
Avoid repeated string atomizations when retrieving the unresolved name of a bound function. r=jandem
Keywords: checkin-needed

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/20aba2431d94
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.