This is an archive of the discontinued LLVM Phabricator instance.

Improve performance TokenizeWindowsCommandLine
ClosedPublic

Authored by takuto.ikuta on Dec 26 2017, 11:10 PM.

Details

Summary

This patch reduces lld link time of chromium's blink_core.dll in component build.
Total size of input argument in .directives become nearly 300MB in the build and calling many strchr and assert becomes bottleneck.

On my desktop machine, 4 times stats of the link time are like below. Improved around 10%.

This patch

TotalSeconds : 13.4918885
TotalSeconds : 13.9474257
TotalSeconds : 13.4941082
TotalSeconds : 13.6077962
Avg : 13.63530465

master

TotalSeconds : 15.6938531
TotalSeconds : 15.7022508
TotalSeconds : 15.9567202
TotalSeconds : 14.5851505
Avg : 15.48449365

Diff Detail

Repository
rL LLVM

Event Timeline

takuto.ikuta created this revision.Dec 26 2017, 11:10 PM
takuto.ikuta edited the summary of this revision. (Show Details)Dec 26 2017, 11:17 PM
takuto.ikuta added reviewers: ruiu, zturner.
takuto.ikuta added a project: lld.
takuto.ikuta added a subscriber: llvm-commits.
ruiu added inline comments.Dec 26 2017, 11:28 PM
llvm/lib/Support/CommandLine.cpp
692 ↗(On Diff #128198)

I'd ignore formfeed and vertical tab, and write like this

return C == ' ' || C = '\t' || C = '\r' || C = '\n';
711 ↗(On Diff #128198)

Please also make the same change for this function.

812 ↗(On Diff #128198)

I don't think you need this comment.

takuto.ikuta edited the summary of this revision. (Show Details)
takuto.ikuta marked 3 inline comments as done.
ruiu added inline comments.Dec 27 2017, 12:11 AM
llvm/lib/Support/CommandLine.cpp
725 ↗(On Diff #128203)

I'd remove this meaningless assignment -- you don't need to minimize the diff, but you want to make the code make sense as a whole for first-time readers.

Removed meaningless assignment.

takuto.ikuta marked an inline comment as done.Dec 27 2017, 12:21 AM
takuto.ikuta added inline comments.
llvm/lib/Support/CommandLine.cpp
725 ↗(On Diff #128203)

I see.

ruiu accepted this revision.Dec 27 2017, 12:27 AM

LGTM

This revision is now accepted and ready to land.Dec 27 2017, 12:27 AM
takuto.ikuta marked an inline comment as done.Dec 27 2017, 12:40 AM

Thank you for review!

I confirmed this passes check-llvm and check-lld.
Can I ask you to merge this again?

This revision was automatically updated to reflect the committed changes.