Patch forthcoming.
Created attachment 251246 [details] Tentative patch, non thread-safe
Comment on attachment 251246 [details] Tentative patch, non thread-safe View in context: https://bugs.webkit.org/attachment.cgi?id=251246&action=review > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2643 > if (Structure* structure = function->allocationStructure()) { We probably need to freeze the rare data here, in case it is removed by the JS thread and then the GC runs at that point. Not sure how to do this, I first need to investigate more deeply how freeze() & the GC works.
Comment on attachment 251246 [details] Tentative patch, non thread-safe View in context: https://bugs.webkit.org/attachment.cgi?id=251246&action=review >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2643 >> if (Structure* structure = function->allocationStructure()) { > > We probably need to freeze the rare data here, in case it is removed by the JS thread and then the GC runs at that point. > Not sure how to do this, I first need to investigate more deeply how freeze() & the GC works. It seems that you're using JSFunction's helpers to get things from the rare data. I would make this explicit here, so instead of function->allocationStructure() you'd do something like rareData->allocationStructure(). And instead of function->allocationProfileWatchpointSet() you'd do rareData->allocationProfileWatchpointSet(). Then you'd do: FunctionRareData* rareData = function->rareData(); if (rareData) { if (Structure* structure = rareData->structure()) { m_graph.freeze(rareData); m_graph.watchpoints().addLazily(rareData->allocationProfileWatchpointSet()); // note that here you don't have to do any null checking - you know that rareData has the InlineWatchpointSet in place rather than via pointer. ... the rest } }
(In reply to comment #3) > Comment on attachment 251246 [details] > Tentative patch, non thread-safe > > View in context: > https://bugs.webkit.org/attachment.cgi?id=251246&action=review > > >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2643 > >> if (Structure* structure = function->allocationStructure()) { > > > > We probably need to freeze the rare data here, in case it is removed by the JS thread and then the GC runs at that point. > > Not sure how to do this, I first need to investigate more deeply how freeze() & the GC works. > > It seems that you're using JSFunction's helpers to get things from the rare > data. I would make this explicit here, so instead of > function->allocationStructure() you'd do something like > rareData->allocationStructure(). And instead of > function->allocationProfileWatchpointSet() you'd do > rareData->allocationProfileWatchpointSet(). I had this discussion with Michael yesterday who told me keeping the accessors would be better, but that is indeed problematic here. I will change that. > > Then you'd do: > > FunctionRareData* rareData = function->rareData(); > if (rareData) { > if (Structure* structure = rareData->structure()) { > m_graph.freeze(rareData); Right, the m_graph.freeze(rareData) was the thing I was not sure would be sufficient (thought I needed to store it somewhere). Shouldn't it rather be: if (rareData) { m_graph.freeze(rareData); if (Structure* structure = rareData->structure()) { ... } } in case the rare data is deallocated in between the two ifs (this shouldn't matter after https://bugs.webkit.org/show_bug.cgi?id=144000 is taken care of)? Thanks!
(In reply to comment #4) > (In reply to comment #3) > > Comment on attachment 251246 [details] > > Tentative patch, non thread-safe > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=251246&action=review > > > > >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2643 > > >> if (Structure* structure = function->allocationStructure()) { > > > > > > We probably need to freeze the rare data here, in case it is removed by the JS thread and then the GC runs at that point. > > > Not sure how to do this, I first need to investigate more deeply how freeze() & the GC works. > > > > It seems that you're using JSFunction's helpers to get things from the rare > > data. I would make this explicit here, so instead of > > function->allocationStructure() you'd do something like > > rareData->allocationStructure(). And instead of > > function->allocationProfileWatchpointSet() you'd do > > rareData->allocationProfileWatchpointSet(). > > I had this discussion with Michael yesterday who told me keeping the > accessors would be better, but that is indeed problematic here. I will > change that. > > > > > Then you'd do: > > > > FunctionRareData* rareData = function->rareData(); > > if (rareData) { > > if (Structure* structure = rareData->structure()) { > > m_graph.freeze(rareData); > > Right, the m_graph.freeze(rareData) was the thing I was not sure would be > sufficient (thought I needed to store it somewhere). > Shouldn't it rather be: > > if (rareData) { > m_graph.freeze(rareData); > if (Structure* structure = rareData->structure()) { > ... > } > } > > in case the rare data is deallocated in between the two ifs (this shouldn't > matter after https://bugs.webkit.org/show_bug.cgi?id=144000 is taken care > of)? It's safe to assume that a GC cannot happen during a DFG phase. The parser counts as a phase. Hence, you only need to freeze it if you decide that you'll be using it after parsing completes - i.e. exactly if/when you addLazily(). > > Thanks!
Created attachment 251254 [details] Patch w/ GC safety
You should rebase your patch. svn up and all.
Created attachment 251256 [details] Rebased patch
Comment on attachment 251256 [details] Rebased patch Clearing flags on attachment: 251256 Committed r183073: <http://trac.webkit.org/changeset/183073>
All reviewed patches have been landed. Closing bug.