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

Possible optimizations - do not merge #638

Closed
wants to merge 37 commits into from

Conversation

chancyk
Copy link
Contributor

@chancyk chancyk commented Jun 10, 2018

I was profiling WeasyPrint and came across a few optimizations that were a 25% speed increase on my particular use case.

3210ea8 simply caches Box.margin_height() because it gets called thousands of times. This is the largest speedup though all three are respectable. Is this safe?

89e269a see if hypothetical_position is already greater than shape.position_y before performing attribute lookups.

71f7415 just assign locals instead of performing 9 different attribute lookups in the original list comprehension.

Thoughts?

Without that, tables with "overflow: auto" don't draw their borders.

Related to Kozea#582.
liZe and others added 7 commits March 27, 2018 22:30
Fix Kozea#586.

For some reason I don't really understand, stripping trailing spaces that would
make the line too long can't be optimized when the space is at the end of the
block. Returning None instead of the index of the last letter should be
possible, but it breaks the rendering when the first letter of the next line
box starts with a character that doesn't allow line breaks after spaces (many
punctuation characters for example) and that doesn't allow line wrap.

This change shouldn't be harmful, as the removed code lines were just cleaning
the resume_at value without changing the logic of line breaking. I suppose that
there's a bug somewhere else.
@Tontyna
Copy link
Contributor

Tontyna commented Jun 27, 2018

To be safe the _cached_margin_height needs a reset to None when any of the 7 involved attributes is changed. Probably requires a crafty Box.__setattr__() -- not being a Python expert I fear this will affect the speed of code, heavily: 7 ifs to execute on every assignment of a Box attribute.

The condition for the short circuit isnt hypothetical_position >= y but hypothetical_position >= y + h -- no chance to avoid calling margin_height().

But wow! 25% speedup by not directly accessing attributes -- didnt know that attribute lookup is that slow in Python. Good to know. There must be many (other, better?) places, especially in the layout code, where local variables could be used to avoid the lookups...

@liZe
Copy link
Member

liZe commented Jun 28, 2018

@chancyk Thanks a lot for your optimizations! I know how hunting speed improvements can sometimes be frustrating and I really appreciate when other people take the time to

To be safe the _cached_margin_height needs a reset to None when any of the 7 involved attributes is changed. Probably requires a crafty Box.setattr() -- not being a Python expert I fear this will affect the speed of code, heavily: 7 ifs to execute on every assignment of a Box attribute.

That's perfectly right! 3210ea8 makes WeasyPrint faster but probably breaks many use cases.

The condition for the short circuit isnt hypothetical_position >= y but hypothetical_position >= y + h -- no chance to avoid calling margin_height().

It's logically true, but in this case h can't be negative, so the hypothetical_position >= y condition is right!

But wow! 25% speedup by not directly accessing attributes -- didnt know that attribute lookup is that slow in Python. Good to know. There must be many (other, better?) places, especially in the layout code, where local variables could be used to avoid the lookups...

Actually, they're not that slow, it's WeasyPrint's fault, here's why (long story, you can skip):


There used to be a class called StyleDict to store CSS attributes and values. This dict-like class had a copy-on-write feature (for memory footprint reasons) and its items could be accessed as attributes (style.width instead of style['width'], for convenience reasons). Style dicts are used everywhere in WeasyPrint and optimizing them is a main way to make WeasyPrint faster.

Dicts in Python are used for everything and are optimized in each Python version (sometimes heavily, like in 3.6, see #70). A simple way to make WeasyPrint really faster was to replace the StyleDict by a real dict, we can get Python's improvements in WeasyPrint for free.

StyleDict has been transformed to inherit from dict, and copy-on-write was removed in #384. It gave amazing results for speed but cost extra memory. This change is already in 0.42.x versions.

That was not enough. Inheriting from dict was a great improvement, but pure dict objects are really better. ad11edf removed the StyleDict class and style items can't be accessed as attributes anymore. This big improvement is in the master branch, but not in 0.x versions.


I've cherry-picked 71f7415, mainly as it avoids calling margin_height() multiple times. As StyleDict objects have been removed, using local variables instead of attribute accesses are not required anymore (at least with the testing documents I've tried).

If you find other optimizations on master, I'll be happy to merge them!

PS: @chancyk I'd be really interested in your performance results using the current master branch with your documents. I'd be happy too to try your samples (if possible), and to know why you use WeasyPrint before releasing the stable version (see #635)!

@liZe liZe closed this Jun 28, 2018
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