Bug 35899 - Switching malloc implementations requires a world rebuild
Summary: Switching malloc implementations requires a world rebuild
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-08 18:57 PST by Geoffrey Garen
Modified: 2010-03-10 15:12 PST (History)
3 users (show)

See Also:


Attachments
patch (2.91 KB, patch)
2010-03-08 18:59 PST, Geoffrey Garen
mjs: review+
Details | Formatted Diff | Diff
patch (4.49 KB, patch)
2010-03-09 21:29 PST, Geoffrey Garen
ap: review+
Details | Formatted Diff | Diff
patch (7.86 KB, patch)
2010-03-10 13:00 PST, Geoffrey Garen
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2010-03-08 18:57:39 PST
...due to recent ValueCheck integration of pointer checking.

Patch coming.
Comment 1 Geoffrey Garen 2010-03-08 18:59:20 PST
Created attachment 50268 [details]
patch
Comment 2 WebKit Review Bot 2010-03-08 19:03:08 PST
Attachment 50268 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/wtf/FastMalloc.cpp:378:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Maciej Stachowiak 2010-03-08 19:13:30 PST
Comment on attachment 50268 [details]
patch

r=me
Comment 4 Alexey Proskuryakov 2010-03-08 19:57:45 PST
+void fastCheckConsistency(const void* p)

I'd give this function a name related to what it does. Something like pointerIsAllocated, perhaps?

Also, the prefix "fast" makes it look like it works with fastMalloc, while it's the opposite that is true.
Comment 5 Geoffrey Garen 2010-03-08 21:01:40 PST
> +void fastCheckConsistency(const void* p)
> 
> I'd give this function a name related to what it does. Something like
> pointerIsAllocated, perhaps?

Right now, the function does the consistency checking, assertions included.

Are you suggesting moving the assertions to the caller?

> Also, the prefix "fast" makes it look like it works with fastMalloc, while it's
> the opposite that is true.

It is the fastMalloc library that does the consistency checking. Whether fastMalloc happens to be using the system malloc or tcmalloc is its own implementation detail.
Comment 6 Alexey Proskuryakov 2010-03-08 23:31:25 PST
> Are you suggesting moving the assertions to the caller?

Perhaps. Or maybe the function could be called somehow else - "consistency" doesn't mean much for a void*.

> It is the fastMalloc library that does the consistency checking.

I didn't know there was such a thing as "fastMalloc library" encompassing all WebKit allocator behaviors. I always thought of it as a particular allocator, and I'm sure some others would be confused, too.
Comment 7 Geoffrey Garen 2010-03-09 10:37:00 PST
> > Are you suggesting moving the assertions to the caller?
> 
> Perhaps. Or maybe the function could be called somehow else - "consistency"
> doesn't mean much for a void*.

OK. Can you suggest something specific?

> > It is the fastMalloc library that does the consistency checking.
> 
> I didn't know there was such a thing as "fastMalloc library" encompassing all
> WebKit allocator behaviors. I always thought of it as a particular allocator,
> and I'm sure some others would be confused, too.

I didn't mean to say that FastMalloc services all WebKit allocations. Indeed, it doesn't. And, therefore, if you use ValueCheck<void*> with a pointer allocated by an allocator that behaves differently than calling fastMalloc/fastFree would, you'll get undefined behavior. But that was true before this patch, too.
Comment 8 Geoffrey Garen 2010-03-09 10:37:32 PST
I'm going to close this bug so it doesn't show up in the commit queue, but we can continue discussing what to do next with it.

http://trac.webkit.org/changeset/55706
Comment 9 Alexey Proskuryakov 2010-03-09 11:35:49 PST
As mentioned above, "isPointerAllocated" or "allocationSize" would be a great thing for a FastMalloc.h to expose. We might even be able implement it for fastMalloc.
Comment 10 Geoffrey Garen 2010-03-09 21:29:08 PST
Created attachment 50369 [details]
patch

Here's an attempt at fixing Alexey's last review comment.
Comment 11 Geoffrey Garen 2010-03-09 21:29:55 PST
Reopening for review.
Comment 12 Alexey Proskuryakov 2010-03-10 01:17:48 PST
Comment on attachment 50369 [details]
patch

It seems that tcmalloc version of fastMallocSize could still use some work. To match OS-provided ones, it should return 0 for pointers to unallocated memory, but there are at least two cases when it doesn't:

1) It will crash if span is null.
2) It disregards low bits of a pointer, so it can mistakenly return a non-zero result.

r=me though, this is already a good improvement. Please add a FIXME if you decide to land as is.
Comment 13 Darin Adler 2010-03-10 09:42:57 PST
Comment on attachment 50369 [details]
patch

> +    const PageID p = reinterpret_cast<uintptr_t>(ptr) >> kPageShift;

Should not use const. Should be named "p", not "page". Should check that the low bits are all zero, as Alexey suggested, and return 0 if they are not.

> +    size_t cl = pageheap->GetSizeClassIfCached(p);
> +    if (cl)
> +        return ByteSizeForClass(cl);

Should be named "sizeClass", not "cl". Should be defined inside the if statement.

> +    Span* span = pageheap->GetDescriptor(p);
> +    ASSERT(span);

Should return 0 if span is 0 rather than asserting, as Alexey suggested. Unless the GetDescriptor function is guaranteed to never return 0, in which case it is appropriate to assert.

> +    cl = span->sizeclass;
> +    if (cl) {

Should be named "sizeClass", not "cl". Should be defined inside the if statement.
Comment 14 Alexey Proskuryakov 2010-03-10 10:33:29 PST
> Should check that the low bits are all zero, as Alexey suggested, and return 0 if they are not.

Is this a correct thing to do? I was somewhat vague in my comment, but I think we'd be returning 0 for all blocks in a page but the first one if we simply checked the bits for being zero.

Right now, the function is guaranteed to return a correct result for allocated pointers, and has undefined behavior otherwise - which is not exactly what malloc_size does, but not too bad either.
Comment 15 Darin Adler 2010-03-10 11:09:08 PST
(In reply to comment #14)
> Is this a correct thing to do? I was somewhat vague in my comment, but I think
> we'd be returning 0 for all blocks in a page but the first one if we simply
> checked the bits for being zero.

Oh, OK. Seems like the correct check should not be too difficult. I didn't read the rest of the FastMalloc code to figure out what it would be.
Comment 16 Geoffrey Garen 2010-03-10 13:00:10 PST
Created attachment 50429 [details]
patch

Turned out not to be too hard to add the improvements Alexey mentioned.
Comment 17 Geoffrey Garen 2010-03-10 13:01:37 PST
> Should not use const. Should be named "p", not "page".
> Should be named "sizeClass", not "cl".

I made the 'if' changes Darin mentioned, but I'd like to keep these names, if that's OK. I agree that they're not optimal, and not in the WebKit coding style, but they match the naming conventions of the rest of the file, which makes text search a lot easier.
Comment 18 WebKit Review Bot 2010-03-10 13:05:42 PST
Attachment 50429 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/wtf/FastMalloc.cpp:4140:  tc_length is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/FastMalloc.cpp:4158:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/wtf/FastMalloc.cpp:4158:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 3 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Sam Weinig 2010-03-10 15:07:33 PST
Comment on attachment 50429 [details]
patch

> +    for (void* free = span->objects; free != NULL; free = *((void**) free))
> +        if (ptr == free)
> +            return 0;

Braces please.

r=me
Comment 20 Geoffrey Garen 2010-03-10 15:12:10 PST
Committed revision 55811.