Bug 156003 – bmalloc: support physical page sizes that don't match the virtual page size (take 2)

• Status: RESOLVED
• Resolution: FIXED
• Component: bmalloc

[Geoffrey Garen]

bmalloc: support physical page sizes that don't match the virtual page size (take 2)

[Geoffrey Garen]

Created attachment 275168 [details]
Patch

[Andreas Kling]

Comment on attachment 275168 [details]
Patch

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

r=me

> Source/bmalloc/bmalloc/VMAllocate.h:55
> +    static size_t cached;
> +    if (!cached)
> +        cached = sysconf(_SC_PAGESIZE);

Is this meaningfully different from
static size_t cached = sysconf(_SC_PAGESIZE);
?

> Source/bmalloc/bmalloc/VMAllocate.h:64
> +    static size_t cached;
> +    if (!cached)
> +        cached = log2(vmPageSize());
> +    return cached;

Same question.

> Source/bmalloc/bmalloc/VMAllocate.h:96
> +    static size_t cached;
> +    if (!cached)
> +        cached = sysconf(_SC_PAGESIZE);
> +    return cached;

Here too.

[Build Bot]

Comment on attachment 275168 [details]
Patch

Attachment 275168 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1066936

New failing tests:
inspector/debugger/breakpoint-condition-with-exception.html
inspector/debugger/breakpoint-eval-with-exception.html

[Build Bot]

Created attachment 275174 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Geoffrey Garen]

> > +    static size_t cached;
> > +    if (!cached)
> > +        cached = sysconf(_SC_PAGESIZE);
> 
> Is this meaningfully different from
> static size_t cached = sysconf(_SC_PAGESIZE);

static will use a separate variable to indicate whether the value has been initialized. So, this is one load fewer. Meh.

[Geoffrey Garen]

(In reply to comment #5)
> > > +    static size_t cached;
> > > +    if (!cached)
> > > +        cached = sysconf(_SC_PAGESIZE);
> > 
> > Is this meaningfully different from
> > static size_t cached = sysconf(_SC_PAGESIZE);
> 
> static will use a separate variable to indicate whether the value has been
> initialized. So, this is one load fewer. Meh.

Better answer: It is not thread-safe to use a separate control variable to indicate that cached has been set. The control variable can appear to be set before cached appears to be set.

[Geoffrey Garen]

mac-debug-ewe failure appears to be a so-called "flaky" test.

Windows build failure is a pre-existing issue: WebCore/CoreGraphicsSPI.h.

mac-wk2-ews failure -- I'm not sure yet.

[Geoffrey Garen]

Committed r198829: <http://trac.webkit.org/changeset/198829>