Bug 170204 – WebAssembly: Worklist should periodically check in to see if there are higher priority jobs to do.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Keith Miller]

This could also happen in Plan.

[Keith Miller]

Created attachment 305777 [details]
Patch

[Keith Miller]

WasmBench shows no regression from this.

[Build Bot]

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


ERROR: Source/JavaScriptCore/runtime/Options.h:514:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 6 files


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

[Saam Barati]

Comment on attachment 305777 [details]
Patch

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

> Source/JavaScriptCore/wasm/WasmPlan.cpp:229
> +    if (UNLIKELY(m_functionLocationInBinary.isEmpty())) {
> +        // This is a weird and rare case. We need to lock here, though, otherwise
> +        // one thread may Complete the Plan while another is moving to Compiled and
> +        // we expect state to be monotonically increasing.
> +        LockHolder locker(m_lock);
> +        if (hasWork())
> +            moveToState(State::Compiled);
> +        return;
> +    }

Why does the below code not gracefully handle this? Seems like it should Just Work

[Mark Lam]

Comment on attachment 305777 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        to it's caller. The main use for this is if a user asynchronously compiles a wasm module

typo: /it's/its/.

[Keith Miller]

(In reply to Saam Barati from comment #4)
> Comment on attachment 305777 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=305777&action=review
> 
> > Source/JavaScriptCore/wasm/WasmPlan.cpp:229
> > +    if (UNLIKELY(m_functionLocationInBinary.isEmpty())) {
> > +        // This is a weird and rare case. We need to lock here, though, otherwise
> > +        // one thread may Complete the Plan while another is moving to Compiled and
> > +        // we expect state to be monotonically increasing.
> > +        LockHolder locker(m_lock);
> > +        if (hasWork())
> > +            moveToState(State::Compiled);
> > +        return;
> > +    }
> 
> Why does the below code not gracefully handle this? Seems like it should
> Just Work

We need to make sure we move to the compiled state before we exit this function if there is no more work. Previously, this worked because if we didn't exit this function until everything was compiled so we would always call complete, which set the state to completed. Now, we need to check on exiting compileFunctions that there is still work so we need to accurately report that the function has finished compiling.

I think there is a simpler version of this code though, where we do hasWork() and moveToState inside the loop. I'll make that change though.

[Keith Miller]

Created attachment 305790 [details]
Patch for landing

[Keith Miller]

Created attachment 305792 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 305792 [details]
Patch for landing

Clearing flags on attachment: 305792

Committed r214566: <http://trac.webkit.org/changeset/214566>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

(In reply to WebKit Commit Bot from comment #9)
> Comment on attachment 305792 [details]
> Patch for landing
> 
> Clearing flags on attachment: 305792
> 
> Committed r214566: <http://trac.webkit.org/changeset/214566>

This change appears to have broken the Windows build: https://build.webkit.org/builders/Apple%20Win%20Release%20(Build)/builds/266

[Keith Miller]

(In reply to Ryan Haddad from comment #11)
> (In reply to WebKit Commit Bot from comment #9)
> > Comment on attachment 305792 [details]
> > Patch for landing
> > 
> > Clearing flags on attachment: 305792
> > 
> > Committed r214566: <http://trac.webkit.org/changeset/214566>
> 
> This change appears to have broken the Windows build:
> https://build.webkit.org/builders/Apple%20Win%20Release%20(Build)/builds/266

Weird, I'll take a look.