Bug 118503 - Improve StringImpl::constructInternal() method
Summary: Improve StringImpl::constructInternal() method
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mikhail Pozdnyakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-07-09 06:45 PDT by Mikhail Pozdnyakov
Modified: 2013-07-12 09:41 PDT (History)
6 users (show)

See Also:


Attachments
patch (5.05 KB, patch)
2013-07-09 07:03 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
alternate patch (2.37 KB, patch)
2013-07-11 01:19 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
same as before (try EWS) (2.37 KB, patch)
2013-07-11 01:48 PDT, Mikhail Pozdnyakov
benjamin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail Pozdnyakov 2013-07-09 06:45:53 PDT
StringImpl::constructInternal does the following  (this code had been located previously inside tryCreateUninitializedForce)

    if (sizeof(CharType) == sizeof(char))
        return adoptRef(new (NotNull, impl) StringImpl(length, Force8BitConstructor));
    return adoptRef(new (NotNull, impl) StringImpl(length));

Compiler has to compile both branches even though optimizer will than remove one of those, and as the function is inline it can affect slightly the compilation time. 
Would be nice to have compile-time polymorphism in the constructor already.
Comment 1 Mikhail Pozdnyakov 2013-07-09 07:03:58 PDT
Created attachment 206316 [details]
patch
Comment 2 Mikhail Pozdnyakov 2013-07-10 14:19:56 PDT
Having had a discussion with Benjamin on IRC came to conclusion that this is not worthwhile as might affect code readability.
Comment 3 Mikhail Pozdnyakov 2013-07-11 01:19:25 PDT
Created attachment 206434 [details]
alternate patch

Solves the problem without affecting code readability.
Comment 4 Mikhail Pozdnyakov 2013-07-11 01:47:01 PDT
EWS doe not run, might be because the bug is closed, reopening..
Comment 5 Mikhail Pozdnyakov 2013-07-11 01:48:44 PDT
Created attachment 206436 [details]
same as before (try EWS)
Comment 6 Benjamin Poulain 2013-07-11 14:11:36 PDT
Comment on attachment 206436 [details]
same as before (try EWS)

I am not sure why you obsess on this branch :)

But I don't see anything wrong with this. Note that the old code handle char and LChar.
Comment 7 Benjamin Poulain 2013-07-11 14:12:02 PDT
Comment on attachment 206436 [details]
same as before (try EWS)

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

> Source/WTF/ChangeLog:22
> +        (WTF::StringImpl::reallocateInternal):
> +        * wtf/text/StringImpl.h:
> +        (WTF::StringImpl::StringImpl):
> +        (WTF::StringImpl::tryCreateUninitialized):
> +        (WTF::LChar):
> +        (WTF::UChar):

Oh, don't forget to fix your changelog.
Comment 8 Mikhail Pozdnyakov 2013-07-12 09:41:22 PDT
Committed r152595: <http://trac.webkit.org/changeset/152595>