Bug 149635 - ParallelHelperPool::runFunctionInParallel() shouldn't allocate, and ParallelHelperPool.h shouldn't be included everywhere
Summary: ParallelHelperPool::runFunctionInParallel() shouldn't allocate, and ParallelH...
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Other
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
Depends on: 149671
Blocks: 149432
  Show dependency treegraph
Reported: 2015-09-29 11:29 PDT by Filip Pizlo
Modified: 2015-09-30 10:14 PDT (History)
3 users (show)

See Also:

the patch (5.08 KB, patch)
2015-09-29 11:32 PDT, Filip Pizlo
saam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2015-09-29 11:29:55 PDT
Patch forthcoming.
Comment 1 Filip Pizlo 2015-09-29 11:32:01 PDT
Created attachment 262084 [details]
the patch
Comment 2 Saam Barati 2015-09-29 11:35:25 PDT
Comment on attachment 262084 [details]
the patch

Comment 3 Filip Pizlo 2015-09-29 13:26:21 PDT
Landed in http://trac.webkit.org/changeset/190324
Comment 4 Alexey Proskuryakov 2015-09-30 10:02:29 PDT
I suspect that this caused flaky crashes, rdar://problem/22916304
Comment 5 WebKit Commit Bot 2015-09-30 10:09:30 PDT
Re-opened since this is blocked by bug 149671
Comment 6 Filip Pizlo 2015-09-30 10:14:11 PDT
(In reply to comment #4)
> I suspect that this caused flaky crashes, rdar://problem/22916304

Yeah, this needs to be rolled out, and I believe that I understand now why the patch is wrong.

The observation behind this patch is that the task cannot run again after runFunctionInParallel returns.  That's true.  But the way that ParallelHelperPool::helperThreadBody works, it will notify runFunctionInParallel that it's no longer going to run the task *before* it derefs the task.  The sequence is:

1) get a task and ref it.
2) run the task.
3) notify task complection.
     ---> at this point, runFunctionInParallel can return.
4) deref the task.
     ---> at this point, the helper thread body will crash if runFunctionInParallel returned.

We could still find other ways to avoid allocating the task.  But I think that it's not worth it if it will be complicated.  The easiest mental model is to treat the task as a proper ref-counted object, in which case a stray RefPtr referencing the task after it cannot run anymore is fine.  If we wanted to allow stack-allocation of the task, then we'd have to change our mental model to: it's illegal to have a RefPtr to the task after we have consensus that the task cannot run anymore.  I don't want to have to think that hard about this code.
Comment 7 Filip Pizlo 2015-09-30 10:14:29 PDT
See https://bugs.webkit.org/show_bug.cgi?id=149635#c6.