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

Performance of 0.8 compared to 0.7.6 #1889

Closed
cryogenian opened this issue Feb 19, 2016 · 12 comments
Closed

Performance of 0.8 compared to 0.7.6 #1889

cryogenian opened this issue Feb 19, 2016 · 12 comments
Assignees
Milestone

Comments

@cryogenian
Copy link

I've just tried to compile current slamdata master with 0.8 psc.

psc@0.7.6 consumes only 390Mb and compiles project in 51sec.

Besides ~3k warnings psc@0.8.0 consumes about 3.9Gb RAM and compiles everything in 2min. This memory consumption definitely will cause travis problems (I'm not sure that compilation time is problem, but 1min is much better than 2min)

@garyb
Copy link
Member

garyb commented Feb 19, 2016

I get something similar: 4.5gb memory usage, 1.32 minutes rather than 28 seconds. Incremental builds are slower too, if I edit SlamData.Notebook.Cell.Common.EvalQuery and rebuild it takes 16s in 0.8 and 9s in 0.7.6.

I wonder how much of that is due to warnings though, the rebuild "only" produces 141, compared to the 2857 for a full build.

@garyb garyb changed the title 3Gb problem Performance of 0.8 compared to 0.7.6 Feb 19, 2016
@paf31 paf31 added this to the 0.9.0 milestone Feb 19, 2016
@nwolverson
Copy link
Contributor

I had a local build where I stripped out all warnings to do some performance measurements, and it made a big difference, I think the version with warnings (about 1500?) was about 3x slower in that case if I recall.

@paf31
Copy link
Contributor

paf31 commented Feb 20, 2016

Perhaps we should add a --no-warnings flag for when this becomes an issue, but I feel like it could be misused.

Another option could be to only print the last N warnings.

What happens when you send stderr to /dev/null?

@felixSchl
Copy link
Contributor

Depending on why warnings are slow, we could maybe:

  • Print warnings as they happen (rather than all at the end)
  • Control the verbosity - split warnings into: deprecations, suggestions etc. and toggle the verbosity on the command line (like gcc's -Wpedantic).
  • Restrict warnings granuarily in the spirit of gcc: -Wnonnull, -Wnonnull-compare and so on

Is it confirmed that the warnings are the main culprit, though? I have also noticed a massive difference in build times when building my docopt project.

@paf31
Copy link
Contributor

paf31 commented Mar 13, 2016

Yes, this is definitely related to warnings. Halogen takes 15s for me, or 6s without warnings.

Now, whether it's creating the string, or printing it, I don't know. I tried replacing [] with DList in MultipleErrors, thinking it was the poor performance of the mappends, but that didn't help.

Perhaps we could take only the last N warnings unless we're in verbose mode or something.

@paf31
Copy link
Contributor

paf31 commented Mar 13, 2016

If I write warnings to a file instead, Halogen takes 14s, but if I use render from boxes instead of the custom renderBox function in the Errors module which strips whitespace, it goes down to 8.7s. So it's basically string manipulation on the warning text which is the issue.

@natefaubion
Copy link
Contributor

FWIW, psa does all the whitespace stripping still, and it's probably a lot faster at it since it's not using a linked list of chars 😆.

@paf31
Copy link
Contributor

paf31 commented Mar 13, 2016

Yeah, we really should be using text, but boxes doesn't, so it's a bit tricky.

We can't really avoid doing the stripping unfortunately, since the output is unreadable without it.

@natefaubion
Copy link
Contributor

Could the formatting combinators we are using be refined so that they don't produce unnecessary padding?

@paf31
Copy link
Contributor

paf31 commented Mar 13, 2016

Sadly not, it's baked into the boxes library, and I get the impression it's quite important to its correctness.

@paf31
Copy link
Contributor

paf31 commented Mar 13, 2016

I think the realistic solution, without adding a flag to turn errors off completely, is to "just" fix the warnings in the core library.

@paf31
Copy link
Contributor

paf31 commented Mar 13, 2016

A compromise could be to only print a warning summary instead of all warnings, controlled by a flag.

@paf31 paf31 self-assigned this Mar 13, 2016
@paf31 paf31 closed this as completed in 6bb8d5e Mar 13, 2016
paf31 added a commit that referenced this issue Mar 13, 2016
Fix #1889, improve performance by avoiding whitespace operations on large strings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants