Bug 229038 – Add for-in OwnStructureMode optimizations to LLInt

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

Add for-in OwnStructureMode optimizations to LLInt

[Keith Miller]

Created attachment 435422 [details]
Patch

[Keith Miller]

Created attachment 435425 [details]
Patch

[Keith Miller]

Created attachment 435427 [details]
Patch

[Mark Lam]

Comment on attachment 435427 [details]
Patch

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

drive by comment.

> Source/JavaScriptCore/runtime/PropertyOffset.h:56
>      UNUSED_PARAM(offset);
>      ASSERT(offset >= invalidOffset);

nit: Might as well make this:
    ASSERT_UNUSED(offset, offset >= invalidOffset);

[Radar WebKit Bug Importer]

<rdar://problem/82126099>

[Keith Miller]

Created attachment 436213 [details]
Patch

[Keith Miller]

Created attachment 436217 [details]
Patch

[Keith Miller]

Created attachment 436219 [details]
Patch

[Keith Miller]

Comment on attachment 435427 [details]
Patch

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

>> Source/JavaScriptCore/runtime/PropertyOffset.h:56
>>      ASSERT(offset >= invalidOffset);
> 
> nit: Might as well make this:
>     ASSERT_UNUSED(offset, offset >= invalidOffset);

I could but maybe not worth changing it unnecessarily for `git blame`

[Saam Barati]

Comment on attachment 436219 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        This patch adds the optimizations we have for OwnStructureMode in
> +        the Baseline to the LLInt. The patch also adds redundant self move

worth saying what those optimizations are right here.

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:3097
> +    # FIXME: This will always call the slow path on at least the first/last execution of EnumeratorNext for any given loop.

Why is that the case? I don't follow.

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:3114
> +    storeVariable(get, m_index, t2, t3)

how does this work? Index isn't tagged here. Seems sketchy that it's a value in the stack. In baseline, you're adding using 64 bit numbers, which seems more legit, and then zexting before using this as an index when accessing the property names array.

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:3131
> +    # FIXME: This should be orb but that doesn't exist for some reason...

file a bug and link it.

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:3149
> +    sxi2q t2, t2

zxi2q seems semantically more correct.

> Source/JavaScriptCore/offlineasm/generate_offset_extractor.rb:99
> +                # Windows complains about signed integers being cast to unsigned but we just want the bits.
> +                outp.puts "\#if COMPILER(MSVC)"
> +                outp.puts "\#pragma warning(disable:4308)"
> +                outp.puts "\#endif"

is this needed?

[Keith Miller]

Comment on attachment 436219 [details]
Patch

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

>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:3097
>> +    # FIXME: This will always call the slow path on at least the first/last execution of EnumeratorNext for any given loop.
> 
> Why is that the case? I don't follow.

Because the first time mode will be InitMode so we'll go to the slow path and the last time because we'll have to call the slow path to figure out if there's GenericMode properties.

>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:3114
>> +    storeVariable(get, m_index, t2, t3)
> 
> how does this work? Index isn't tagged here. Seems sketchy that it's a value in the stack. In baseline, you're adding using 64 bit numbers, which seems more legit, and then zexting before using this as an index when accessing the property names array.

Yeah, that seems like a bug. I guess no one looks at the high bits of the value? I'll change the above addi to addq.

>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:3131
>> +    # FIXME: This should be orb but that doesn't exist for some reason...
> 
> file a bug and link it.

Done.

>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:3149
>> +    sxi2q t2, t2
> 
> zxi2q seems semantically more correct.

Yeah, I think the max inline property capacity is pretty small but I'll change it.

>> Source/JavaScriptCore/offlineasm/generate_offset_extractor.rb:99
>> +                outp.puts "\#endif"
> 
> is this needed?

Yeah, MSVC complains that we try to multiply a signed and unsigned value, which is a really dumb warning... It's also why I had to cast the sizeof(EncodedJSValue) for the first out of line offset computation.

[Keith Miller]

Created attachment 436300 [details]
Patch

[Saam Barati]

Comment on attachment 436300 [details]
Patch

r=me

[EWS]

Committed r281523 (240896@main): <https://commits.webkit.org/240896@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 436300 [details].

[Michael Catanzaro]

Hi, this broke the cloop build. (I guess we don't have any cloop EWS.)

[1/127] Generating ../../JavaScriptCore/DerivedSources/LLIntAssembly.h
FAILED: JavaScriptCore/DerivedSources/LLIntAssembly.h 
cd /home/mcatanzaro/Projects/WebKit/WebKitBuild/JSCOnly/JavaScriptCore/DerivedSources && /usr/bin/cmake -E env CMAKE_CXX_COMPILER_ID=GNU GCC_OFFLINEASM_SOURCE_MAP=OFF /usr/bin/ruby /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb -I/home/mcatanzaro/Projects/WebKit/WebKitBuild/JSCOnly/JavaScriptCore/DerivedSources/ /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/llint/LowLevelInterpreter.asm /home/mcatanzaro/Projects/WebKit/WebKitBuild/JSCOnly/bin/LLIntOffsetsExtractor /home/mcatanzaro/Projects/WebKit/WebKitBuild/JSCOnly/JavaScriptCore/DerivedSources/LLIntAssembly.h normal && /usr/bin/cmake -E touch_nocreate /home/mcatanzaro/Projects/WebKit/WebKitBuild/JSCOnly/JavaScriptCore/DerivedSources/LLIntAssembly.h
/home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/cloop.rb:652:in `lowerC_LOOP': undefined method `uint32MemRef' for #<Immediate:0x000056431ed48a20 @codeOrigin=#<CodeOrigin:0x000056431b395aa8 @sourceFile=#<SourceFile:0x000056431b01dfb0 @name=#<Pathname:/home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm>, @fileNumber=3>, @lineNumber=3111>, @value=32> (due to /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:3111) (LoweringError)
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/backends.rb:134:in `lower'
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/backends.rb:192:in `block in lower'
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/backends.rb:190:in `each'
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/backends.rb:190:in `lower'
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:413:in `block (5 levels) in <main>'
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:107:in `inAsm'
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:412:in `block (4 levels) in <main>'
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/settings.rb:210:in `emitCodeInConfiguration'
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:410:in `block (3 levels) in <main>'
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/settings.rb:107:in `forSettings'
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:397:in `block (2 levels) in <main>'
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:393:in `each'
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:393:in `block in <main>'
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:384:in `open'
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:384:in `<main>'
/home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/cloop.rb:652:in `lowerC_LOOP': undefined method `uint32MemRef' for #<Immediate:0x000056431ed48a20 @codeOrigin=#<CodeOrigin:0x000056431b395aa8 @sourceFile=#<SourceFile:0x000056431b01dfb0 @name=#<Pathname:/home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm>, @fileNumber=3>, @lineNumber=3111>, @value=32> (NoMethodError)
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/backends.rb:134:in `lower'
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/backends.rb:192:in `block in lower'
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/backends.rb:190:in `each'
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/backends.rb:190:in `lower'
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:413:in `block (5 levels) in <main>'
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:107:in `inAsm'
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:412:in `block (4 levels) in <main>'
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/settings.rb:210:in `emitCodeInConfiguration'
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:410:in `block (3 levels) in <main>'
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/settings.rb:107:in `forSettings'
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:397:in `block (2 levels) in <main>'
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:393:in `each'
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:393:in `block in <main>'
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:384:in `open'
	from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:384:in `<main>'
ninja: build stopped: subcommand failed.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 229494

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #15)
> /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/cloop.rb:
> 652:in `lowerC_LOOP': undefined method `uint32MemRef' for
> #<Immediate:0x000056431ed48a20 @codeOrigin=#<CodeOrigin:0x000056431b395aa8
> @sourceFile=#<SourceFile:0x000056431b01dfb0
> @name=#<Pathname:/home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/
> llint/LowLevelInterpreter64.asm>, @fileNumber=3>, @lineNumber=3111>,
> @value=32> (due to
> /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/llint/
> LowLevelInterpreter64.asm:3111) (LoweringError)

I don't understand any of this code, so I decided to do a rollout for now, sorry. Does the error message make sense to you? I don't see uint32MemRef anywhere in the diff, but I've double-checked and the error is definitely caused by this commit.

[Keith Miller]

(In reply to Michael Catanzaro from comment #17)
> (In reply to Michael Catanzaro from comment #15)
> > /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/cloop.rb:
> > 652:in `lowerC_LOOP': undefined method `uint32MemRef' for
> > #<Immediate:0x000056431ed48a20 @codeOrigin=#<CodeOrigin:0x000056431b395aa8
> > @sourceFile=#<SourceFile:0x000056431b01dfb0
> > @name=#<Pathname:/home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/
> > llint/LowLevelInterpreter64.asm>, @fileNumber=3>, @lineNumber=3111>,
> > @value=32> (due to
> > /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/llint/
> > LowLevelInterpreter64.asm:3111) (LoweringError)
> 
> I don't understand any of this code, so I decided to do a rollout for now,
> sorry. Does the error message make sense to you? I don't see uint32MemRef
> anywhere in the diff, but I've double-checked and the error is definitely
> caused by this commit.

It's because the code in the LowLevelInterpreter*.asm is fed to a script which turns it into an inline assembly C++ file. I see the issue though. I'm a bit surprised it didn't cause any breakage though O.o

[Keith Miller]

(In reply to Keith Miller from comment #18)
> (In reply to Michael Catanzaro from comment #17)
> > (In reply to Michael Catanzaro from comment #15)
> > > /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/cloop.rb:
> > > 652:in `lowerC_LOOP': undefined method `uint32MemRef' for
> > > #<Immediate:0x000056431ed48a20 @codeOrigin=#<CodeOrigin:0x000056431b395aa8
> > > @sourceFile=#<SourceFile:0x000056431b01dfb0
> > > @name=#<Pathname:/home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/
> > > llint/LowLevelInterpreter64.asm>, @fileNumber=3>, @lineNumber=3111>,
> > > @value=32> (due to
> > > /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/llint/
> > > LowLevelInterpreter64.asm:3111) (LoweringError)
> > 
> > I don't understand any of this code, so I decided to do a rollout for now,
> > sorry. Does the error message make sense to you? I don't see uint32MemRef
> > anywhere in the diff, but I've double-checked and the error is definitely
> > caused by this commit.
> 
> It's because the code in the LowLevelInterpreter*.asm is fed to a script
> which turns it into an inline assembly C++ file. I see the issue though. I'm
> a bit surprised it didn't cause any breakage though O.o

Welp, figured it out. It's because all the code in the enumerator_next fast path wasn't running. I was comparing the stack address of m_mode with 2 which will always be false and take the slow path. Whoops.

[Keith Miller]

Created attachment 436395 [details]
Patch for landing

[EWS]

Committed r281565 (240930@main): <https://commits.webkit.org/240930@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 436395 [details].