Bug 200862 - Use makeString and multi-argument StringBuilder::append instead of less efficient multiple appends
Summary: Use makeString and multi-argument StringBuilder::append instead of less effic...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on: 200921
Blocks:
  Show dependency treegraph
 
Reported: 2019-08-17 14:03 PDT by Darin Adler
Modified: 2019-08-22 09:27 PDT (History)
5 users (show)

See Also:


Attachments
Patch (61.56 KB, patch)
2019-08-17 16:35 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (3.65 MB, application/zip)
2019-08-17 17:39 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews115 for mac-highsierra (3.88 MB, application/zip)
2019-08-17 18:16 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews215 for win-future (13.97 MB, application/zip)
2019-08-17 18:28 PDT, EWS Watchlist
no flags Details
Patch (61.65 KB, patch)
2019-08-18 08:18 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-highsierra (3.26 MB, application/zip)
2019-08-18 09:24 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews115 for mac-highsierra (3.51 MB, application/zip)
2019-08-18 10:16 PDT, EWS Watchlist
no flags Details
Patch (69.74 KB, patch)
2019-08-21 19:05 PDT, Darin Adler
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2019-08-17 14:03:27 PDT
Use makeString and multi-argument StringBuilder::append instead of less efficient multiple appends
Comment 1 Darin Adler 2019-08-17 16:35:58 PDT Comment hidden (obsolete)
Comment 2 EWS Watchlist 2019-08-17 17:39:40 PDT Comment hidden (obsolete)
Comment 3 EWS Watchlist 2019-08-17 17:39:41 PDT Comment hidden (obsolete)
Comment 4 EWS Watchlist 2019-08-17 18:16:29 PDT Comment hidden (obsolete)
Comment 5 EWS Watchlist 2019-08-17 18:16:31 PDT Comment hidden (obsolete)
Comment 6 EWS Watchlist 2019-08-17 18:28:24 PDT Comment hidden (obsolete)
Comment 7 EWS Watchlist 2019-08-17 18:28:27 PDT Comment hidden (obsolete)
Comment 8 EWS Watchlist 2019-08-17 18:44:17 PDT Comment hidden (obsolete)
Comment 9 Darin Adler 2019-08-18 08:18:14 PDT Comment hidden (obsolete)
Comment 10 EWS Watchlist 2019-08-18 09:24:28 PDT Comment hidden (obsolete)
Comment 11 EWS Watchlist 2019-08-18 09:24:30 PDT Comment hidden (obsolete)
Comment 12 EWS Watchlist 2019-08-18 10:16:26 PDT Comment hidden (obsolete)
Comment 13 EWS Watchlist 2019-08-18 10:16:27 PDT Comment hidden (obsolete)
Comment 14 EWS Watchlist 2019-08-18 10:25:36 PDT Comment hidden (obsolete)
Comment 15 Sam Weinig 2019-08-18 15:32:30 PDT
Comment on attachment 376639 [details]
Patch

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

> Source/WebCore/css/CSSFontFaceRule.cpp:53
>      StringBuilder result;

This is unused and can be removed.

> Source/WebCore/css/CSSFontFaceSrcValue.cpp:77
>          result.appendLiteral("local(");
>      else
>          result.appendLiteral("url(");
> -    result.append(m_resource);
> -    result.append(')');
> -    if (!m_format.isEmpty()) {
> -        result.appendLiteral(" format(");
> -        result.append(m_format);
> -        result.append(')');
> -    }
> +    result.append(m_resource, ')');

It might read a little nicer to put literals ("local(" or "url(") as a ternary expression for the first parameter.

> Source/WebCore/css/CSSGradientValue.cpp:941
>                  result.appendLiteral("color-stop(");
>                  result.appendFixedPrecisionNumber(position);
> -                result.appendLiteral(", ");
> -                result.append(stop.m_color->cssText());
> -                result.append(')');
> +                result.append(", ", stop.m_color->cssText(), ')');

If you wanted to, you could collapse all the appends here doing, result.append("color-stop(", FormattedNumber::fixedPrecision(position), ", ", stop.m_color->cssText(), ')').
Comment 16 Darin Adler 2019-08-18 16:37:36 PDT
Looks like the test failures indicate a failure of StringBuilder::append!
Comment 17 Darin Adler 2019-08-18 16:47:23 PDT
(In reply to Darin Adler from comment #16)
> Looks like the test failures indicate a failure of StringBuilder::append!

Yes, I think I see a bug in StringBuilder::appendForAdapters when the string is 8-bit, the thing to be appended is 16-bit, and the 8-bit buffer is big enough to hold the new length. There's no call to allocateBufferUpConvert in that case, yet there needs to be.
Comment 18 Darin Adler 2019-08-18 16:56:05 PDT
Comment on attachment 376639 [details]
Patch

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

I’ll upload a new patch after I fix the bug in StringBuilder::append.

>> Source/WebCore/css/CSSFontFaceRule.cpp:53
>>      StringBuilder result;
> 
> This is unused and can be removed.

Done.

>> Source/WebCore/css/CSSFontFaceSrcValue.cpp:77
>> +    result.append(m_resource, ')');
> 
> It might read a little nicer to put literals ("local(" or "url(") as a ternary expression for the first parameter.

Done, and then converted the function to makeString instead of StringBuilder.

>> Source/WebCore/css/CSSGradientValue.cpp:941
>> +                result.append(", ", stop.m_color->cssText(), ')');
> 
> If you wanted to, you could collapse all the appends here doing, result.append("color-stop(", FormattedNumber::fixedPrecision(position), ", ", stop.m_color->cssText(), ')').

Yes, happy to do that. I think there’s quite a bit more to collapse elsewhere too; I was working on doing more and more of this and it got a little "out of hand" and so I stopped myself and wanted to land what I had.

Done.
Comment 19 Darin Adler 2019-08-18 16:56:53 PDT
(In reply to Darin Adler from comment #17)
> (In reply to Darin Adler from comment #16)
> > Looks like the test failures indicate a failure of StringBuilder::append!
> 
> Yes, I think I see a bug in StringBuilder::appendForAdapters when the string
> is 8-bit, the thing to be appended is 16-bit, and the 8-bit buffer is big
> enough to hold the new length. There's no call to allocateBufferUpConvert in
> that case, yet there needs to be.

Sadly, in the case where the test was failing the string doesn't even need to be 16-bit, but one of the substrings happens to be allocated as 16-bit even though all the characters in it are Latin-1.
Comment 20 Darin Adler 2019-08-18 17:20:09 PDT
Comment on attachment 376639 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/shared/IDBDatabaseInfo.cpp:166
>          builder.append(objectStore.loggingString(1));
>          builder.append('\n');

Should probably combine these two also.
Comment 21 Darin Adler 2019-08-20 09:05:32 PDT
Fixing the StringBuilder::appendForAdapters bug in bug 200921, and then will post a new patch for this bug so EWS can show us the tests succeeding.
Comment 22 Darin Adler 2019-08-21 19:05:48 PDT
Created attachment 376963 [details]
Patch
Comment 23 Ryosuke Niwa 2019-08-21 21:03:10 PDT
Comment on attachment 376963 [details]
Patch

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

> Source/JavaScriptCore/runtime/TypeSet.cpp:416
> +            representation.append(StringView { field.get() }, ", ");

Do we need to explicitly create StringView here?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:114
> +

This seems unrelated not that it matters either way.

> Tools/WebKitTestRunner/TestController.cpp:1101
> +    response = makeString(TestController::webProcessName(), ": ", webContentPID, '\n',
> +        TestController::networkProcessName(), ": ", networkingPID, '\n');

Should we just call dumpResponse instead?

> Tools/WebKitTestRunner/TestController.cpp:1103
> -    builder.append('\n');
> +    response = "\n"_s;

And here?
Comment 24 Darin Adler 2019-08-22 09:02:32 PDT
I can’t figure out what the Windows build failure is or if it was caused by this patch.

I don’t know why most EWS results are missing. Maybe "New EWS" isn’t working at all at this time?
Comment 25 Darin Adler 2019-08-22 09:26:12 PDT
Committed r249013: <https://trac.webkit.org/changeset/249013>
Comment 26 Radar WebKit Bug Importer 2019-08-22 09:27:45 PDT
<rdar://problem/54601523>