Bug 157956 – JSScope::abstractAccess doesn't need to copy the SymbolTableEntry, it can use it by reference

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

Copying a FatEntry is slow because we have to malloc then free the FatEntry

[Saam Barati]

Created attachment 279502 [details]
patch

[Geoffrey Garen]

Comment on attachment 279502 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=279502&action=review

> Source/JavaScriptCore/runtime/JSScope.cpp:75
> +            if (!entry.isNull()) {
> +                op = ResolveOp(makeType(ClosureVar, needsVarInjectionChecks), depth, 0, lexicalEnvironment, entry.watchpointSet(), entry.scopeOffset().offset());
>                  return true;
>              }
> +
> +            if (scope->type() == ModuleEnvironmentType) {

I think this logic changed.

entry.isNull() is only possible in the get() world. In the find world, the equivalent is iter == end(). So, I think the return above needs to be unconditional, and this module code needs to be outside the iter != end() check.

[Saam Barati]

(In reply to comment #2)
> Comment on attachment 279502 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279502&action=review
> 
> > Source/JavaScriptCore/runtime/JSScope.cpp:75
> > +            if (!entry.isNull()) {
> > +                op = ResolveOp(makeType(ClosureVar, needsVarInjectionChecks), depth, 0, lexicalEnvironment, entry.watchpointSet(), entry.scopeOffset().offset());
> >                  return true;
> >              }
> > +
> > +            if (scope->type() == ModuleEnvironmentType) {
> 
> I think this logic changed.
> 
> entry.isNull() is only possible in the get() world. In the find world, the
> equivalent is iter == end(). So, I think the return above needs to be
> unconditional, and this module code needs to be outside the iter != end()
> check.

Good catch.
In general, my change assumes that the following could be true: (iter != end() && entry.isNull())
I think this is a bad assumption though. I believe that
(iter != end()) implies (!entry.isNull())

[Saam Barati]

Created attachment 279512 [details]
patch

[WebKit Commit Bot]

Attachment 279512 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSScope.cpp:62:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 279513 [details]
patch

fix style

[Geoffrey Garen]

Comment on attachment 279513 [details]
patch

r=me

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/201235

[Csaba Osztrogonác]

(In reply to comment #8)
> landed in:
> http://trac.webkit.org/changeset/201235

It made 300 tests assert on all debug bots, see build.webkit.org for details.

[Saam Barati]

(In reply to comment #9)
> (In reply to comment #8)
> > landed in:
> > http://trac.webkit.org/changeset/201235
> 
> It made 300 tests assert on all debug bots, see build.webkit.org for details.

Thanks. I'm looking into it now.

[Saam Barati]

I'm pretty sure I've found the bug.

[Saam Barati]

(In reply to comment #11)
> I'm pretty sure I've found the bug.

Running debug tests locally now

[Saam Barati]

(In reply to comment #12)
> (In reply to comment #11)
> > I'm pretty sure I've found the bug.
> 
> Running debug tests locally now

Fixed debug assertion failures in:
http://trac.webkit.org/changeset/201266