Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Speed up parsing @args.rsp compiler arguments #26554

Merged
merged 1 commit into from
Jun 19, 2017
Merged

Conversation

orivej
Copy link
Contributor

@orivej orivej commented Jun 13, 2017

Improves upon #25205.

https://gist.github.com/pbogdan/9d6986bf931b58a70d75e14eb40ee8a1 parsing time is
reduced from one minute to one second.

@pbogdan
Copy link
Member

pbogdan commented Jun 13, 2017

Thank you! I started a build for a Haskell project to test this PR. Being a big rebuild it will likely take a while to complete but will post results once it finishes (unless this gets merged first).

@orivej orivej force-pushed the rsp branch 2 times, most recently from b7ada0e to e635a0d Compare June 14, 2017 11:41
@pbogdan
Copy link
Member

pbogdan commented Jun 14, 2017

I seem to be getting a failure building ghc-7.10.3 https://gist.github.com/pbogdan/9f40ebd95cdbbba3e7408ac18c8327a4 😕 Can't debug further right now but looks like something related to treatment of arguments enclosed within escaped quotes.

@pbogdan
Copy link
Member

pbogdan commented Jun 14, 2017

I straced the failing command, abbreviated output below:

My current reading of what's happening is:

  1. GHC gets invoked with "inplace/bin/ghc-stage1" ... -optc-DProjectVersion=\"7.10.3\" ... meaning ProjectVersion macro should evaluate to "7.10.3" with the quotes included (https://stackoverflow.com/questions/26213071/change-a-string-macro-at-compilation-time)

  2. This gets translated to "-DProjectVersion=\"7.10.3\"" within the generated rsp file.

  3. utils.sh translates that entry into -DProjectVersion="7.10.3" (note lack of escaped quotes).

  4. As RtsUtils.c uses ProjectVersion as a string so it must contain the quotes:

    static void mkRtsInfoPair(char *key, char *val) {
        /* XXX should check for "s, \s etc in key and val */
        printf(" ,(\"%s\", \"%s\")\n", key, val);
    }
    ....
    void printRtsInfo(void) {
        ...
        mkRtsInfoPair("GHC version",             ProjectVersion);
    

    so the compilation fails.

@orivej
Copy link
Contributor Author

orivej commented Jun 14, 2017

Thank you! I had to add -r to read, now it should work.

@pbogdan
Copy link
Member

pbogdan commented Jun 14, 2017

I have another build running with the updated PR. It's unlikely to finish before I head to bed so might take some time before I can report back.

@pbogdan
Copy link
Member

pbogdan commented Jun 15, 2017

The following test build succeeded. Built on top of master @ 90cf4cf.

The timings for building just the Haskell project once all dependencies are built:

before:

real    19m26.297s
user    19m5.064s
sys     1m20.928s

after:

real    2m16.866s
user    2m50.204s
sys     0m44.228s

That's on a project with 13 modules in the library component, 3 executables, a dummy test suite and sizeable dependency footprint.
Test was done in a VM but even on my desktop some of the rsp files involved would take close to 3 minutes to parse which is now down to 2.5s 😄

@domenkozar
Copy link
Member

domenkozar commented Jun 17, 2017

I don't know how many times the wrapper is called, but it's still in almost two orders of magnitude slower than in release-17.03 branch (using the above gist as a test).

rsp branch:

$ source pkgs/build-support/cc-wrapper/utils.sh
$ time expandResponseParams @blabla.rsp

real    0m0.848s
user    0m0.834s
sys     0m0.012s

release-17.03 branch:

$ source pkgs/build-support/cc-wrapper/utils.sh
$ time expandResponseParams @blabla.rsp

real    0m0.019s
user    0m0.018s
sys     0m0.001s

@orivej
Copy link
Contributor Author

orivej commented Jun 17, 2017

It certainly is slower than parsing with eval in the release:

args=$(<"${p:1}")
eval 'for arg in '${args//$/\\$}'; do params+=("$arg"); done'

which incorrectly parses some files with $ and \. (Correct parsing is documented here. In fact this workaround fixed some cases and broke the one that I noticed in #25205.)

@domenkozar
Copy link
Member

I think we should merge this and open an issue to rewrite the parsing in C++.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants