Bug 146874

Summary: Watchpoints should be allocated with FastMalloc
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, basile_clement, benjamin, ggaren, mark.lam, mhahnenb, mmirman, msaboff, nrotem, oliver, saam, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
the patch mitz: review+

Description Filip Pizlo 2015-07-10 22:46:39 PDT
Previously we had some cruft about allocating some kinds of Watchpoints in SegmentedVectors.  That's pretty sad.  Also, it appears that some of the watchpoints are allocated with normal malloc.  Also sad.
Comment 1 Filip Pizlo 2015-07-10 22:49:21 PDT
Created attachment 256645 [details]
the patch
Comment 2 Filip Pizlo 2015-07-10 22:51:09 PDT
Comment on attachment 256645 [details]
the patch

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

> Source/JavaScriptCore/bytecode/CodeBlockJettisoningWatchpoint.h:2
> - * Copyright (C) 2013 Apple Inc. All rights reserved.
> + * Copyright (C) 2013, 2015 Apple Inc. All rights reserved.

Oops, I'll revert.  I just deleted code.
Comment 3 Filip Pizlo 2015-07-10 22:54:24 PDT
Landed in http://trac.webkit.org/changeset/186705
Comment 4 Darin Adler 2015-07-11 15:30:19 PDT
Comment on attachment 256645 [details]
the patch

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

> Source/JavaScriptCore/dfg/DFGDesiredWatchpoints.h:95
>          typename HashSet<WatchpointSetType*>::iterator iter = m_sets.begin();
>          typename HashSet<WatchpointSetType*>::iterator end = m_sets.end();
> -        for (; iter != end; ++iter) {
> -            common.watchpoints.append(CodeBlockJettisoningWatchpoint(codeBlock));
> -            Adaptor::add(codeBlock, *iter, &common.watchpoints.last());
> -        }
> +        for (; iter != end; ++iter)
> +            Adaptor::add(codeBlock, *iter, common.watchpoints.add(codeBlock));

Here’s another way to write this:

    for (auto* set : m_sets)
        Adaptor::add(codeBlock, set, common.watchpoints.add(codeBlock));

I think the modern for loop makes this code look clearer.
Comment 5 Darin Adler 2015-07-11 15:30:57 PDT
Comment on attachment 256645 [details]
the patch

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

>> Source/JavaScriptCore/dfg/DFGDesiredWatchpoints.h:95
>> +            Adaptor::add(codeBlock, *iter, common.watchpoints.add(codeBlock));
> 
> Here’s another way to write this:
> 
>     for (auto* set : m_sets)
>         Adaptor::add(codeBlock, set, common.watchpoints.add(codeBlock));
> 
> I think the modern for loop makes this code look clearer.

Just noticed you did a variation on that in your very next patch. Never mind!