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

See Also:

Attachments
the patch (5.08 KB, patch)
2015-09-29 11:32 PDT, Filip Pizlo 		saam: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

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

r=me
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.