Bug 95793 – Improve the initialization of empty strings

• Status: NEW
• Component: Web Template Framework

[Benjamin Poulain]

In many cases, emptyString() is not generating code as compact as it should be.

[Benjamin Poulain]

Created attachment 162154 [details]
Patch

[Eric Seidel (no email)]

I'm confused.  This just seems to be making emptyString() more expensive?  When is this a win?

[Eric Seidel (no email)]

Hmm... You attempt to explain in the ChangeLog.  Perhaps in my tired state I just don't understand. :)  will try re-reading tomorrow.

[Benjamin Poulain]

(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.

[Benjamin Poulain]

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.

[Benjamin Poulain]

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.

[Darin Adler]

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?

[Darin Adler]

(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?

[Eric Seidel (no email)]

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. :)

[Benjamin Poulain]

(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.

[Eric Seidel (no email)]

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. :)

[Ahmad Saleem]

I checked via bugID on Webkit GitHub and noted that this r+ patch didn't landed. Do we need this now? Thanks!

[Tim Nguyen (:ntim)]

(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.