Bug 95793 - Improve the initialization of empty strings
Summary: Improve the initialization of empty strings
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-04 16:03 PDT by Benjamin Poulain
Modified: 2022-10-09 08:55 PDT (History)
16 users (show)

See Also:


Attachments
Patch (48.07 KB, patch)
2012-09-04 21:21 PDT, Benjamin Poulain
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2012-09-04 16:03:39 PDT
In many cases, emptyString() is not generating code as compact as it should be.
Comment 1 Benjamin Poulain 2012-09-04 21:21:39 PDT
Created attachment 162154 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-09-04 21:46:00 PDT
I'm confused.  This just seems to be making emptyString() more expensive?  When is this a win?
Comment 3 Eric Seidel (no email) 2012-09-04 21:46:57 PDT
Hmm... You attempt to explain in the ChangeLog.  Perhaps in my tired state I just don't understand. :)  will try re-reading tomorrow.
Comment 4 Benjamin Poulain 2012-09-04 22:03:03 PDT
(In reply to comment #3)
> Hmm... You attempt to explain in the ChangeLog.  Perhaps in my tired state I just don't understand. :)  will try re-reading tomorrow.

I probably need a better ChangeLog if that is not clear :)

So previously if you did:
    String foo = emptyString();

Because the return type was const String&, it was really:
    String foo;
    const String& temp = emptyString();
    if (!temp.isNull())
        temp.ref();
    foo.m_impl <= temp; // assignment, no ref :)


The ideas here are:
1) Get rid of the branch, just ref().
2) Do that in the function, not on the call site.

This is why the code is so much more compact for the callers with the patch. Compared to String(""), it is only one less instruction.
Comment 5 Benjamin Poulain 2012-09-05 00:48:38 PDT
Comment on attachment 162154 [details]
Patch

Stricto sensu, this is relying on a compiler optimization.

I should make the solution explicit with rvalue reference, and that will also solve the problem of operator=.

I'll make an update tomorrow.
Comment 6 Benjamin Poulain 2012-09-06 14:32:01 PDT
Comment on attachment 162154 [details]
Patch

> I should make the solution explicit with rvalue reference, and that will also solve the problem of operator=.

I tried that, it does not make any difference for the compiler. I am pleasantly surprised how smart the compiler is with this.
Using r-value adds risks and complexity, so I prefer the current version.

Putting the original patch back for review.
Comment 7 Darin Adler 2013-01-15 15:40:41 PST
Comment on attachment 162154 [details]
Patch

When do we ever need this emptyString() rather than emptyStringRef(). Can’t we just change emptyString() to do exactly what emptyStringRef() does?
Comment 8 Darin Adler 2013-01-15 15:41:52 PST
(In reply to comment #7)
> When do we ever need this emptyString() rather than emptyStringRef(). Can’t we just change emptyString() to do exactly what emptyStringRef() does?

Oh, I see you discussing that with Eric. This seems really tricky. It’s hard to know when to use each function. Is there a better solution?
Comment 9 Eric Seidel (no email) 2013-01-15 16:51:16 PST
Comment on attachment 162154 [details]
Patch

It's difficult for me to believe that these will be used consistently correctly.  But overall this patch looks like a good thing.  I think I'd prefer if we just had emptyString() which returns a ref, but if you believe this to be a real perf/size win, then it's worth it. You should obviously be aware that your hard-work of tuning is very likely to be stale immediately. :)
Comment 10 Benjamin Poulain 2013-01-15 17:28:46 PST
(In reply to comment #9)
> (From update of attachment 162154 [details])
> It's difficult for me to believe that these will be used consistently correctly.  But overall this patch looks like a good thing.  I think I'd prefer if we just had emptyString() which returns a ref, but if you believe this to be a real perf/size win, then it's worth it. You should obviously be aware that your hard-work of tuning is very likely to be stale immediately. :)

Thanks Eric.

I am sure you are right, this can easily go badly.
I'll rebase this when I am back working on memory improvement for ARM.
Comment 11 Eric Seidel (no email) 2013-01-15 17:46:05 PST
You might also consider naming emptyString() to emptyStringByValue() and using it only in the couple cases which matter, otherwise the normal emptyString() is a ref and is the "expected" function for the naive contributor to use. :)
Comment 12 Ahmad Saleem 2022-10-09 05:51:17 PDT
I checked via bugID on Webkit GitHub and noted that this r+ patch didn't landed. Do we need this now? Thanks!
Comment 13 Tim Nguyen (:ntim) 2022-10-09 08:55:49 PDT
(In reply to Ahmad Saleem from comment #12)
> I checked via bugID on Webkit GitHub and noted that this r+ patch didn't
> landed. Do we need this now? Thanks!

Chris has landed some changes that covers this area, you can check if each individual file in the patch was covered, and post a new patch if not.