All users were logged out of Bugzilla on October 13th, 2018

SetFunctionNameIfNoOwnName should try to avoid resolve and define operations

RESOLVED FIXED in Firefox 61

Status

()

P3
normal
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: anba, Assigned: anba)

Tracking

(Blocks: 1 bug, {triage-deferred})

unspecified
mozilla61
triage-deferred
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

a year ago
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);
(Assignee)

Updated

a year ago
Blocks: 1307395
Keywords: triage-deferred
Priority: -- → P3
(Assignee)

Comment 1

7 months ago
Created attachment 8960205 [details] [diff] [review]
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.
Attachment #8960205 - Flags: review?(jorendorff)
(Assignee)

Comment 2

7 months ago
Created attachment 8960212 [details] [diff] [review]
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.
Attachment #8960212 - Flags: review?(jorendorff)
(Assignee)

Comment 3

7 months ago
Created attachment 8960220 [details] [diff] [review]
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?
Attachment #8960220 - Flags: review?(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.
Attachment #8960205 - Flags: review?(jorendorff) → review+
Attachment #8960212 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 5

7 months ago
(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 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.
Attachment #8960220 - Flags: review?(jorendorff) → review+
(Assignee)

Updated

7 months ago
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
(Assignee)

Comment 7

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

Comment 8

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

Updated

7 months ago
Duplicate of this bug: 1427228
(Assignee)

Comment 10

7 months ago
Created attachment 8960958 [details] [diff] [review]
bug1371591-part1-mark-anon-in-parser.patch

Updated per review comments, carrying r+.
Attachment #8960205 - Attachment is obsolete: true
Attachment #8960958 - Flags: review+
(Assignee)

Comment 11

7 months ago
Created attachment 8960960 [details] [diff] [review]
bug1371591-part3-store-atom.patch

Update part 3 to apply on updated part 1.
Attachment #8960220 - Attachment is obsolete: true
Attachment #8960960 - Flags: review+

Comment 13

7 months ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 14

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/068989e6ca33
https://hg.mozilla.org/mozilla-central/rev/c5be9e57bbef
https://hg.mozilla.org/mozilla-central/rev/312e1b24fca5
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.