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

Split 'go' into with/without attrs code paths #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mikehat
Copy link

@mikehat mikehat commented May 29, 2015

This is the first of two pull requests that do some internal rewiring. This first change improves BigTable performance considerably with no negative effect on other benchmarks.

bigTable/Utf8 runs 25% faster.

If the BigTable benchmark is a usefull test, this is not a trivial improvement. It might make the Blaze benchmarks page even more impressive.

The change is simple: begin renderXXX with the assumption that an element has no attributes, then proceed with the original algorithm when encountering an Add(Custom)?Attribute.

As mentioned above, this suggestion precedes another pull request that will yield almost (within 3%) identical performance while promoting CSS styles and classes to the same syntactic sweetness as attributes. I initially split the go function because I couldn't find any other way to make my CSS additions an 'optional' package. I thought I'd just make them an optional part of the core
algorithm. It turns out that the same technique (thinking of attributes as an 'optional' feature) has a pleasant effect on the original algorithm.

I have one lingering question (ans: see next comment) about the benchmark results. Why does this modification have such a drastic effect on the Utf8 benchmarks (including wideTable and basic) and not the others? I'll guess that the reason is hiding somewhere in GHC's optimizations, but it would be nice to know. I'd expect to see a similar decrease in execution time across the other modules (in ms, not %).

@mikehat
Copy link
Author

mikehat commented May 29, 2015

Answered my own question. But there's more to the answer than I expected...

By importing the blaze-builder files into my src/ dir, the benchmarks now behave in the expected way. By 'expected,' I mean that splitting go into go and go_attrs results in a small improvement in speed; the benchmark improved mostly because I added blaze-builder sources to my src/ directory. The funny thing here is that it turns out I that I got this result by building RunHtmlBenchmarks with the wrong version of blaze-builder! The Hackage page points to the wrong repository, which gave me much better benchmark results than the latest versions do.

I next built RunHtmlBenchmarks again, using sources from the latest blaze-builder and bytestring-builder packages (I am running ghc 7.6.3). Predictably, it is as slow as builds using the cabal install-ed packages. I can only conclude that the whole bytestring-builder/blaze-builder dependency chain has slowed down considerably since it was split out of blaze-builder.

This whole issue is separate from the CSS additions I'm suggesting. By proposing this go-attrs branch, I was only trying to show that my benchmark improvements were not specifically related to the code in my css branch. In the process, I uncovered a pretty big drop in benchmark performance caused by recent changes in blaze-markup's dependencies.

In case you can't tell by looking at the changes I made in the renderXXX functions, here's where I think the slowdown is occurring. Eliminating the attrs parameter in go until an attribute is actually encountered causes the big improvement in the bigTable/Utf8 benchmark. The attrs == mempty :: Builder in the mappend attrs statements in go's Custom?(Parent|Leaf) patterns is doing more work than the implied nothing-at-all that mappend mempty would suggest. I've tried using Maybe Builder for the attrs param with similarly faster-but-uglier results (see my maybe-attrs branch). I can't find any specific difference between the older blaze-builder code and the newer bytestring-builder code, but there is a new dependency: deepseq.

So, I'll stand by by assertion that this branch is actually 25% faster than the current master branch at bigTable/Utf8. But it may be more interesting to say that the master branch has slowed down by 33% due to changes in blaze-markdown's dependencies.

If you want to talk offline, send me an email.

@mikehat
Copy link
Author

mikehat commented Jun 8, 2015

I made a repo demonstrating the above issue at blaze-all. That repo is just an attempt to document a few patches. Please read its README.

The branches in there will let you test blaze-markup built against different blaze-builder versions and see the resulting benchmarks. Also, the css branch brings the changes made to both blaze-markup and blaze-html into one place with some sample code.

@mikehat
Copy link
Author

mikehat commented Jun 10, 2015

I upgraded to GHC 7.8 and platform 2014.2.

The wide discrepancy between the bigTable/Utf8 performance is now gone, and all benchmarks are now slower on my machine. As I can't build against the blaze-markup-0.3.3.4 sources any more, no comparisons can be made.

This go-attrs branch is slightly faster than the master branch. This is the predicted result, so I'll return my focus to the css branch and pull request. That branch is still a few percent faster than the current master.

It is worth noting, though, that compiling everything with the latest platform runs the bigTable/Utf8 benchmarks 20% slower than a year ago on my machine. This may be an unfortunate consequence of changes to the bytestring..blaze-markup dependency chain. Ultimately, speed is really more of a bragging-rights issue than a functional one. On that thought, I'll go back to coding with CSS-enhanced blaze-html...

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.

1 participant