Bug 89608 – FastMalloc.cpp needs to have its style updated

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebCore Misc.

[Alex Christensen]

Source/WTF/wtf/FastMalloc.cpp does not conform to webkit coding style.  This is causing problems with reordering the definitions so I can fix a 0-length array compile error.

[Alex Christensen]

Created attachment 148670 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 148670 [details]
Patch

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

> Source/WTF/wtf/FastMalloc.cpp:246
> -    header->m_size = n;
> +    header->msize = n;

m_size is a member, right? In that case, it's correct that it's prefixed by m_.

> Source/WTF/wtf/FastMalloc.cpp:682
> -// Mapping from size to size_class and vice versa
> +// Mapping from size to sizeclass and vice versa

It should probably be sizeClass instead.

[Ryosuke Niwa]

Comment on attachment 148670 [details]
Patch

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

> Source/WTF/wtf/FastMalloc.cpp:246
> -    header->m_size = n;
> +    header->msize = n;

m_size is a member, right? In that case, it's correct that it's prefixed by m_.

> Source/WTF/wtf/FastMalloc.cpp:682
> -// Mapping from size to size_class and vice versa
> +// Mapping from size to sizeclass and vice versa

It should probably be sizeClass instead.

> Source/WTF/wtf/FastMalloc.cpp:1040
> +                char* newallocation = reinterpret_cast<char*>(MetaDataAlloc(kAllocIncrement));

newAllocation.

[Ryosuke Niwa]

Comment on attachment 148670 [details]
Patch

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

> Source/WTF/wtf/FastMalloc.cpp:1409
> +    PageMap pagemap;
> +    mutable PageMapCache pagemapcache;

pageMap, pageMapCache.

> Source/WTF/wtf/FastMalloc.cpp:1727
> +        } else
> +            // Keep looking in larger classes
> +            continue;
> +

These two lines should have curly brackets.

> Source/WTF/wtf/FastMalloc.cpp:1768
> +    for (Span* span = large.normal.next;
> +            span != &large.normal;
> +            span = span->next) {

It seems like this can all fit in one line.

> Source/WTF/wtf/FastMalloc.cpp:1782
> +    for (Span* span = large.returned.next;
> +            span != &large.returned;
> +            span = span->next) {

Ditto.

> Source/WTF/wtf/FastMalloc.cpp:1995
> +                                    static_cast<size_t>(s->length << kPageShift));

Wrong indentation. It should be indented 4 spaces to the right of TCMalloc_SystemRelease.
(i.e. we don't align subsequent indentations).

> Source/WTF/wtf/FastMalloc.cpp:2230
> +                                static_cast<size_t>(s->length << kPageShift));

Ditto about wrong indentation.

> Source/WTF/wtf/FastMalloc.cpp:2297
> +    void PushRange(int N, void *start, void *end)

Nit: void* instead of void *.

> Source/WTF/wtf/FastMalloc.cpp:2303
> +    void PopRange(int N, void **start, void **end)

Ditto.

> Source/WTF/wtf/FastMalloc.cpp:2370
> +    int freelistlength(size_t cl) const
> +    {
> +        return list[cl].length();
> +    }

You can keep this a one-line function. We allow f() { return ~ }. In fact, it's probably preferable here because the definition is so trivial.

> Source/WTF/wtf/FastMalloc.cpp:2379
> +    void Deallocate(void* ptr, size_t sizeclass);

Useless argument name ptr.

> Source/WTF/wtf/FastMalloc.cpp:2388
> +    bool SampleAllocation(size_t k);

Ditto about k.

> Source/WTF/wtf/FastMalloc.cpp:2391
> +    void PickNextSample(size_t k);

Ditto.

> Source/WTF/wtf/FastMalloc.cpp:2425
> +    void InsertRange(void *start, void *end, int);

Nit: void* instead of void *.

> Source/WTF/wtf/FastMalloc.cpp:2428
> +    void RemoveRange(void **start, void **end, int*);

Ditto.

> Source/WTF/wtf/FastMalloc.cpp:2962
> +         ptr = nptr;

Nit: extra space.

> Source/WTF/wtf/FastMalloc.cpp:3011
> +    for (size_t cl = 0; cl < kNumClasses; ++cl)
> +        if (list[cl].length() > 0)
> +            ReleaseToCentralCache(cl, list[cl].length());

This "for" needs curly brackets because it contains two lines.

> Source/WTF/wtf/FastMalloc.cpp:3113
> +    // increment is "sample_period/2".

Nit: spaces around /.

> Source/WTF/wtf/FastMalloc.cpp:3122
> +        for (i = 0; i < (static_cast<int>(sizeof(primesList) / sizeof(primesList[0])) - 1); i++)
> +            if (primesList[i] >= flagValue)
> +                break;

Ditto about curly brackets.

> Source/WTF/wtf/FastMalloc.cpp:3332
> +    if (GetThreadHeap() == heap)
> +        // Somehow heap got reinstated by a recursive call to malloc
> +        // from pthread_setspecific. We give up in this case.
> +        return;

We need to keep curly brackets for this. In WebKit style, we decide whether we use curly brackets or not based on the number of physical lines, not the number of statements.

> Source/WTF/wtf/FastMalloc.cpp:3429
> +            if (classCount)
> +                for (size_t cl = 0; cl < kNumClasses; ++cl)
> +                    classCount[cl] += h->freelistlength(cl);

Ditto about curly brackets.

> Source/WTF/wtf/FastMalloc.cpp:3579
> +                    - stats.threadBytes
> +                    - stats.centralBytes
> +                    - stats.pageheapBytes;

Wrong indentation. "- stats." should be 4 spaces to the right of "*value".

> Source/WTF/wtf/FastMalloc.cpp:3593
> +            SpinLockHolder ll(&pageheapLock);

Why did you rename l to ll?

> Source/WTF/wtf/FastMalloc.cpp:3625
> +            SpinLockHolder ll(&pageheapLock);

Ditto.

> Source/WTF/wtf/FastMalloc.cpp:3785
> +    } else
> +        // The common case, and also the simplest. This just pops the
> +        // size-appropriate freelist, afer replenishing it if it's empty.
> +        ret = CheckedMallocResult(heap->Allocate(size));

Keep the curly brackets here as well.

> Source/WTF/wtf/FastMalloc.cpp:-3687
> +    if (!ret)
>  #ifdef WTF_CHANGES
> -    if (crashOnFailure) // This branch should be optimized out by the compiler.
> -        CRASH();
> +        if (crashOnFailure) // This branch should be optimized out by the compiler.
> +            CRASH();
>  #else
> -    errno = ENOMEM;
> +        errno = ENOMEM;
>  #endif
> -  }
> -  return ret;

Ditto.

> Source/WTF/wtf/FastMalloc.cpp:3864
> +        cl++;

Missing indentation.

> Source/WTF/wtf/FastMalloc.cpp:3939
> +    info.fsmblks   = static_cast<int>(stats.threadBytes
> +                                    + stats.centralBytes
> +                                    + stats.transferBytes);

Wrong indentation.

> Source/WTF/wtf/FastMalloc.cpp:3946
> +                                    - stats.threadBytes
> +                                    - stats.centralBytes
> +                                    - stats.transferBytes
> +                                    - stats.pageheapBytes);
> +

Ditto.

> Source/WTF/wtf/FastMalloc.cpp:4033
> +void* fastCalloc(size_t n, size_t elemsize)

Should be elemSize instead.

> Source/WTF/wtf/FastMalloc.cpp:4042
> +TryMallocReturnValue tryFastCalloc(size_t n, size_t elemsize)

Dotto.

> Source/WTF/wtf/FastMalloc.cpp:4054
> +void* calloc(size_t n, size_t elemsize)

Ditto.

> Source/WTF/wtf/FastMalloc.cpp:4100
> +void* fastRealloc(void* oldptr, size_t newSize)

Nit: s/oldptr/oldPtr/

> Source/WTF/wtf/FastMalloc.cpp:4112
> +TryMallocReturnValue tryFastRealloc(void* oldptr, size_t newSize)

Ditto.

> Source/WTF/wtf/FastMalloc.cpp:4127
> +void* realloc(void* oldptr, size_t newSize)

Ditto.

> Source/WTF/wtf/FastMalloc.cpp:4179
> +        void* newptr = doMalloc(newSize);

Nit: s/newptr/newPtr/

> Source/WTF/wtf/FastMalloc.cpp:4315
> +extern "C" int posix_memalign(void** resultptr, size_t align, size_t size)

Nit: s/resultptr/resultPtr/

[Alex Christensen]

Created attachment 148814 [details]
Patch

[Alexey Proskuryakov]

It is intentional that FastMalloc retains its original style. Perhaps we no longer need that, but someone who actively maintains this file needs to make the call whether it's OK to update the style.

[Eric Seidel (no email)]

My understanding is our version of fastmalloc is now a very old fork of tcmalloc.

[Alex Christensen]

(In reply to comment #7)
> My understanding is our version of fastmalloc is now a very old fork of tcmalloc.

If it's a very old fork, is there any reason we shouldn't update the style?  64-bit Windows won't work until this is resolved.

[Brent Fulgham]

I would like to see this patch landed, in support of getting 64-bit Windows builds working.  Who is responsible for this file?

[Ryosuke Niwa]

Comment on attachment 148814 [details]
Patch

I'd say we should go ahead and land this. If it becomes a problem, we can always roll them back.

[WebKit Review Bot]

Comment on attachment 148814 [details]
Patch

Rejecting attachment 148814 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
 (offset 30 lines).
Hunk #64 succeeded at 4720 (offset 28 lines).
Hunk #65 succeeded at 4742 (offset 28 lines).
Hunk #66 succeeded at 4770 (offset 28 lines).
Hunk #67 succeeded at 4800 (offset 28 lines).
Hunk #68 succeeded at 4824 (offset 28 lines).
3 out of 68 hunks FAILED -- saving rejects to file Source/WTF/wtf/FastMalloc.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Ryosuke Ni..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/14828093

[Brent Fulgham]

Alex e-mailed me a while ago that he was not doing more WebKit work, and was abandoning these changes.  I'll revise this patch and resubmit.

[Darin Adler]

(In reply to comment #8)
> 64-bit Windows won't work until this is resolved.

Why tie that to a style change?

[Brent Fulgham]

Created attachment 173960 [details]
Patch

[Darin Adler]

Comment on attachment 173960 [details]
Patch

Why is the 64-bit Windows support mixed in with this style change to the entire file.

[Brent Fulgham]

(In reply to comment #15)
> (From update of attachment 173960 [details])
> Why is the 64-bit Windows support mixed in with this style change to the entire file.

It isn't.  This change is only style modifications.

The original bug report (see https://bugs.webkit.org/show_bug.cgi?id=89366) indicated that the changes to support a 64-bit build could not be done cleanly due to style bot rejecting the necessary reordering of the file.

Another option would be to willfully ignore the style bot and just push the reordering change through.  Then we could close this issue as WONTFIX.

But is there a downside to making this file match our expected style, assuming the tests pass?

[Maciej Stachowiak]

(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 173960 [details] [details])
> > Why is the 64-bit Windows support mixed in with this style change to the entire file.
> 
> It isn't.  This change is only style modifications.
> 
> The original bug report (see https://bugs.webkit.org/show_bug.cgi?id=89366) indicated that the changes to support a 64-bit build could not be done cleanly due to style bot rejecting the necessary reordering of the file.
> 
> Another option would be to willfully ignore the style bot and just push the reordering change through.  Then we could close this issue as WONTFIX.
> 
> But is there a downside to making this file match our expected style, assuming the tests pass?

You should ignore the stylebot for <https://bugs.webkit.org/show_bug.cgi?id=89366>. Stylebot should not be an obstacle to reordering.

This style change may or may not be a good idea. But it would be good to ask people who have recently or frequently worked with FastMalloc what they think.

[Brent Fulgham]

I have updated https://bugs.webkit.org/show_bug.cgi?id=89366 to work against ToT.  This is a reordering-only change that is necessary to support the template specialization to be landed as part of https://bugs.webkit.org/show_bug.cgi?id=88344.

[Brent Fulgham]

I'm closing this bug as WONTFIX.  The maintenance burden of tracking this separately from upstream just to satisfy the stylebot seems like a waste of time.

[Brent Fulgham]

Comment on attachment 173960 [details]
Patch

Clear flags to get rid of this in the queue.

[Oliver Hunt]

(In reply to comment #19)
> I'm closing this bug as WONTFIX.  The maintenance burden of tracking this separately from upstream just to satisfy the stylebot seems like a waste of time.

Honestly i think at this point we've diverged so much, and upstream is now completely different I think we may as well bring fastmalloc up to webkit style.

[Maciej Stachowiak]

Yeah, I'm not sure upstream is relevant to us, but there would still be a cost when trying to SVN annotate.