Bug 94562

Summary: Store CString data in the CStringBuffer to avoid the double indirection
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: Web Template FrameworkAssignee: Benjamin Poulain <benjamin>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, gyuyoung.kim, jchaffraix, kling, peter+ews, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 94871    
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from gce-cr-linux-01
none
Patch
none
Patch darin: review+

Description Benjamin Poulain 2012-08-20 19:21:54 PDT
Making CString more efficient...
Comment 1 Benjamin Poulain 2012-08-20 19:36:45 PDT
Created attachment 159592 [details]
Patch
Comment 2 WebKit Review Bot 2012-08-20 20:17:35 PDT
Comment on attachment 159592 [details]
Patch

Attachment 159592 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13549197
Comment 3 Peter Beverloo (cr-android ews) 2012-08-20 20:22:40 PDT
Comment on attachment 159592 [details]
Patch

Attachment 159592 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13549200
Comment 4 Benjamin Poulain 2012-08-20 20:27:16 PDT
Created attachment 159603 [details]
Patch
Comment 5 Benjamin Poulain 2012-08-20 20:33:44 PDT
Once again Chromium has issues with WebKitTestAPI.

I skipped the test as usual. I email Julien Chaffraix about the issue.
Comment 6 WebKit Review Bot 2012-08-20 22:30:48 PDT
Comment on attachment 159603 [details]
Patch

Attachment 159603 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13546271

New failing tests:
accessibility/aria-checkbox-checked.html
animations/3d/matrix-transform-type-animation.html
http/tests/appcache/cyrillic-uri.html
canvas/philip/tests/2d.canvas.readonly.html
animations/animation-add-events-in-handler.html
http/tests/appcache/deferred-events-delete-while-raising.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
accessibility/anchor-linked-anonymous-block-crash.html
http/tests/appcache/deferred-events-delete-while-raising-timer.html
accessibility/aria-disabled.html
http/tests/appcache/crash-when-navigating-away-then-back.html
animations/animation-border-overflow.html
animations/animation-direction-alternate-reverse.html
accessibility/accessibility-object-detached.html
http/tests/appcache/abort-cache-ondownloading-manifest-404.html
http/tests/appcache/credential-url.html
http/tests/appcache/access-via-redirect.php
animations/3d/change-transform-in-end-event.html
animations/animation-controller-drt-api.html
animations/3d/transform-perspective.html
accessibility/accessibility-node-reparent.html
http/tests/appcache/deferred-events.html
animations/3d/state-at-end-event-transform.html
animations/3d/transform-origin-vs-functions.html
accessibility/accessibility-node-memory-management.html
animations/animation-css-rule-types.html
animations/animation-direction-reverse-fill-mode-hardware.html
accessibility/adjacent-continuations-cause-assertion-failure.html
accessibility/aria-help.html
Comment 7 WebKit Review Bot 2012-08-20 22:30:57 PDT
Created attachment 159622 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 8 Benjamin Poulain 2012-08-21 11:27:55 PDT
Created attachment 159726 [details]
Patch
Comment 9 Benjamin Poulain 2012-08-21 11:28:47 PDT
(In reply to comment #6)
> (From update of attachment 159603 [details])
> Attachment 159603 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/13546271

I could not reproduce any of the Chromium failure. I uploaded the same patch again, maybe the bot was sick?
Comment 10 Benjamin Poulain 2012-08-21 12:18:39 PDT
Comment on attachment 159726 [details]
Patch

Ok, Chromium uses CStringBuffer in WebCString. I missed that.
I'll update.
Comment 11 Benjamin Poulain 2012-08-21 12:31:04 PDT
Created attachment 159738 [details]
Patch
Comment 12 Darin Adler 2012-08-21 13:43:01 PDT
Comment on attachment 159738 [details]
Patch

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

> Source/WTF/wtf/text/CString.cpp:37
> +    if (length > (numeric_limits<size_t>::max() - sizeof(CStringBuffer) / sizeof(char)))
> +        CRASH();

sizeof(char) is defined as 1 by the C and C++ language. It's usually considered better style to just omit it.

> Source/WTF/wtf/text/CString.cpp:41
> +    size_t size = sizeof(CStringBuffer) + length * sizeof(char);

Same comment about sizeof(char).

> Source/WTF/wtf/text/CString.cpp:57
> +    if (!str)
> +        return;

I probably would have put an ASSERT(!length) in there too.

> Source/WTF/wtf/text/CString.h:35
> +// CStringBuffer is the ref-counted storage class for the characters of CStringBuffer.

Should be "for the characters in a CString", not "for the characters of CStringBuffer".

> Source/WTF/wtf/text/CString.h:36
> +// The data is implicitely allocated 1 character longer than length(), as it is zero-terminated.

the word implicitly does not have the letter ā€œeā€

> Source/WTF/wtf/text/CString.h:49
> +    CStringBuffer(size_t length)
> +        : m_length(length)
> +    { }

This is not quite the usual style. Normally we put everything on one line, or everything on different lines. It's unusual to have { } on one line with everything else on separate lines.

I also suggest marking this constructor explicit.

> Source/WTF/wtf/text/CString.h:52
> +    size_t m_length;

I think this should be defined const.
Comment 13 Benjamin Poulain 2012-08-21 14:58:25 PDT
Committed r126191: <http://trac.webkit.org/changeset/126191>