1371591 - SetFunctionNameIfNoOwnName should try to avoid resolve and define operations

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[André Bargull [:anba]]

SetFunctionNameIfNoOwnName currently always calls the resolve-hook and defines a new slot for the "name" property. We should try to avoid both calls, maybe we can simply stash the function name in the existing JSFunction::atom_ field?


This µ-benchmark improved from 760ms to 130ms when I simply stored the function name in JSFunction::atom_:

    var t = Date.now();
    for (var i=0;i<1000000;++i) {
        var o = {["a"]: function (){}};
    }
    print(Date.now() - t);

Comment 1

[André Bargull [:anba]]

Attachment: bug1371591-part1-mark-anon-in-parser.patch

First some clean-ups and simplifications:

Parser.cpp, SyntaxParseHandler.h, FullParseHandler.h, BytecodeEmitter.cpp:
- Only call checkAndSetIsDirectRHSAnonFunction() if the left-hand side is definitely an unparenthesised identifier. That way we don't need to check this condition in the BytecodeEmitter and it will simplify a change for part 3.

BytecodeEmitter::setOrEmitSetFunName()
- Add an extra assertion for clarity that this function should only be called if the function/class is marked as |isDirectRHSAnonFunction()|.

NameResolver::resolve():
- Add null-pointer checks for ParseNodeKind::Name and ParseNodeKind::Function, so we can remove the |cur == nullptr| check at the top of resolve(). All other callers already check for null-pointers before recursively calling resolve().
- Update the checks for ParseNodeKind::For{In,Of} (pn_kid1 is never a null-pointer, pn_kid2 is always a null-pointer).
- Update some comments and fix indentation.

Comment 2

[André Bargull [:anba]]

Attachment: bug1371591-part2-reduce-indirections.patch

- Remove a straight out lie in the IdToFunctionName() doc-comment.
- Change SetFunctionNameIfNoOwnName to skip the Value->Id conversion and instead directly construct the function name from a Symbol or Atom value.
- And replace the HasOwnProperty() call with |NativeObject::contains()|, so we don't run the resolve-handler for class constructors.

Comment 3

[André Bargull [:anba]]

Attachment: bug1371591-part3-store-atom.patch

The actual change to store the function name in JSFunction::atom_ to avoid creating a "name" property:
- Replace |CompileTimeName| with |InferredName| in various methods/fields because we no longer only set the inferred name at compile-time.
- Remove the empty name assertion in JSFunction::getUnresolvedName(), because this case is now possible: |var o = {[""]: class {}};|.
- Remove HAS_COMPILE_TIME_NAME/HAS_INFERRED_NAME from JSFunction::STABLE_ACROSS_CLONES, because it may not be set in class constructor functions: |var c = class {static [randomOneOf("name", "no-name")](){}};|. (It's correct to remove it because of that case, right?)
- Don't assign a guessed function name in NameResolve::resolveFun() if we know that this function will get a (dynamically computed) inferred name at runtime.


And now most important:
Do you think this change is correct or is there some subtle reason why we can't save the function name in JSFunction::atom_ for dynamically computed names?

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 8960205 [details] [diff] [review]
bug1371591-part1-mark-anon-in-parser.patch

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

r=me and I sure hope these comments are helpful!

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5488,1 @@
>      if (maybeFun->isKind(ParseNodeKind::Function)) {

The only other possibility is ParseNodeKind::Class, right? Would you mind asserting that after this if-block?

::: js/src/frontend/FullParseHandler.h
@@ +672,5 @@
>      }
>  
> +    bool isUnparenthesizedNameForRHSAnonFunction(ParseNode* pn) {
> +        return pn->isKind(ParseNodeKind::Name) && !pn->isInParens();
> +    }

If possible, please delete these two methods (checkAndSetIsDirectRHSAnonFunction and isUnparenthesizedNameForRHSAnonFunction) from the ParseHandler interface, and handle this case in FPH::newAssignment(), addPropertyDefinition(), and newExportDefaultDeclaration(). (FPH can have private methods for any shared code. I'm not worried about the extra branch per local variable declaration; if you are, add a method newAssignmentWithoutCheckingForSetFunctionName.)

Rationale: make the Parser code read better; make it harder for BinAST to get this bit wrong; eliminate the rather subtle "this boolean doesn't matter because that other method is a no-op" comment in SyntaxParseHandler; concentrate responsibility for this bit's correctness in one class (FPH).

The ParseHandler interface is already a bit of a dumping ground for complexity, and it's in constant danger of getting worse. To fight this, we should try to make it syntax-driven and applicative (in the functional-programming sense). In a perfect world, it would be nothing but `newFoo()` methods, one for each spec grammar production, with no setters at all.

::: js/src/frontend/NameFunctions.cpp
@@ +540,5 @@
>            case ParseNodeKind::Import:
>            case ParseNodeKind::ExportFrom:
>            case ParseNodeKind::ExportDefault:
>              MOZ_ASSERT(cur->isArity(PN_BINARY));
> +            // The left halves of Import and ExportFrom don't contain any

Heh. Now the comment makes the reader wonder, "But what about ExportDefault?"

I guess I would change it back, but it's up to you.

Comment 5

[André Bargull [:anba]]

(In reply to Jason Orendorff [:jorendorff] from comment #4)
> ::: js/src/frontend/NameFunctions.cpp
> @@ +540,5 @@
> >            case ParseNodeKind::Import:
> >            case ParseNodeKind::ExportFrom:
> >            case ParseNodeKind::ExportDefault:
> >              MOZ_ASSERT(cur->isArity(PN_BINARY));
> > +            // The left halves of Import and ExportFrom don't contain any
> 
> Heh. Now the comment makes the reader wonder, "But what about ExportDefault?"
> 
> I guess I would change it back, but it's up to you.

ExportDefault can contain arbitrary expressions (for the "export default AssignExpression" form). That's why I wanted to exclude it from the "[...] don't contain any unconstrained expressions [...]" comment.

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 8960220 [details] [diff] [review]
bug1371591-part3-store-atom.patch

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

Yes, I think this change is correct.

Function names are a mess, before and after this patch. :-( It seems sloppy that we use explicitName() in so many case and displayAtom() in so many others, generally with no indication of whether it matters at all which one we're using. I wonder if we have bugs where displayAtom() is used on bound functions, producing different results depending on whether getUnresolvedName() has been called. Or other stuff like that.

::: js/src/vm/JSFunction.cpp
@@ +1332,3 @@
>          // Unnamed class expressions should not get a .name property at all.
>          if (name)
>              v.set(name);

The `if (name)` test is unnecessary. If you agree, please remove it, and set v unconditionally.

Comment 7

[André Bargull [:anba]]

(In reply to Jason Orendorff [:jorendorff] from comment #6)
> Comment on attachment 8960220 [details] [diff] [review]
> bug1371591-part3-store-atom.patch
> 
> Review of attachment 8960220 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yes, I think this change is correct.
> 
> Function names are a mess, before and after this patch. :-( It seems sloppy
> that we use explicitName() in so many case and displayAtom() in so many
> others, generally with no indication of whether it matters at all which one
> we're using. I wonder if we have bugs where displayAtom() is used on bound
> functions, producing different results depending on whether
> getUnresolvedName() has been called. Or other stuff like that.

Yeah, bug 1371593 and the new bug 1447362 both made displayAtom() for bound functions a bit unreliable. But even before that displayAtom() no longer gave consistent results, for example because of syntax parsing:
---
function testDisplayName(directive) {
    eval(`
    function outer() {
        "${directive}";
        function inner() {
            return function(){};
        }
        print(displayName(inner()));
    }
    outer();
    `);
}

// Prints "inner/<" with syntax parsing enabled.
testDisplayName();

// Prints "outer/inner/<" with syntax parsing disabled.
testDisplayName("use asm");
---

And nobody seems to care. :-)


> ::: js/src/vm/JSFunction.cpp
> @@ +1332,3 @@
> >          // Unnamed class expressions should not get a .name property at all.
> >          if (name)
> >              v.set(name);
> 
> The `if (name)` test is unnecessary. If you agree, please remove it, and set
> v unconditionally.

bug 1447362 removes this test.

Comment 8

[André Bargull [:anba]]

(In reply to Jason Orendorff [:jorendorff] from comment #4)
> If possible, please delete these two methods
> (checkAndSetIsDirectRHSAnonFunction and
> isUnparenthesizedNameForRHSAnonFunction) from the ParseHandler interface,
> and handle this case in FPH::newAssignment(), addPropertyDefinition(), and
> newExportDefaultDeclaration(). (FPH can have private methods for any shared
> code. I'm not worried about the extra branch per local variable declaration;
> if you are, add a method newAssignmentWithoutCheckingForSetFunctionName.)

FPH::newAssignment() and FPH::newExportDefaultDeclaration() are easy to change, but FPH::addPropertyDefinition() is a tricky one, because we need to check for anonymous functions before calling FoldConstants [2], but also want to call FoldConstants before marking the object literal as non-constant [2] resp. calling addPropertyDefinition().


[1] https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/js/src/frontend/Parser.cpp#9536-9537
[2] https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/js/src/frontend/Parser.cpp#9562-9563

Comment 10

[André Bargull [:anba]]

Attachment: bug1371591-part1-mark-anon-in-parser.patch

Updated per review comments, carrying r+.

Comment 11

[André Bargull [:anba]]

Attachment: bug1371591-part3-store-atom.patch

Update part 3 to apply on updated part 1.

Comment 12

[André Bargull [:anba]]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/068989e6ca33
Part 1: Move parenthesized identifier check to parser. r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5be9e57bbef
Part 2: Remove extra calls in SetFunctionNameIfNoOwnName. r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/312e1b24fca5
Part 3: Store dynamic function names into the name atom instead of a property. r=jorendorff

Comment 14

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/068989e6ca33
https://hg.mozilla.org/mozilla-central/rev/c5be9e57bbef
https://hg.mozilla.org/mozilla-central/rev/312e1b24fca5