Bug 143999 – Remove AllocationProfileWatchpoint node

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Basile Clement]

Patch forthcoming.

[Basile Clement]

Created attachment 251246 [details]
Tentative patch, non thread-safe

[Basile Clement]

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.

[Filip Pizlo]

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
    }
}

[Basile Clement]

(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!

[Filip Pizlo]

(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!

[Basile Clement]

Created attachment 251254 [details]
Patch w/ GC safety

[Filip Pizlo]

You should rebase your patch.  svn up and all.

[Basile Clement]

Created attachment 251256 [details]
Rebased patch

[WebKit Commit Bot]

Comment on attachment 251256 [details]
Rebased patch

Clearing flags on attachment: 251256

Committed r183073: <http://trac.webkit.org/changeset/183073>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.