Bug 35833 – Clean IntRect

• Status: RESOLVED
• Resolution: INVALID
• Component: New Bugs

[Benjamin Poulain]

IntRect is not as light as I would like it to be. When rendering some content, ~1% of the time is spent jumping to IntRect::intersects(). This function could be made inline.

[Benjamin Poulain]

Created attachment 50155 [details]
Inline + cleaning

Make IntRect::intersects() inline.
Remove IntRect::IntRect(const FloatRect) because I cannot find it used anywhere.
Compile scale(float) only if there is the dashboard support

I really only care about intersects() being inline. Removing the two unused functions is just because I am tired of people complaining of the size of Webkit... ;)

[WebKit Review Bot]

Attachment 50155 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/343437

[WebKit Review Bot]

Attachment 50155 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/342477

[Benjamin Poulain]

Comment on attachment 50155 [details]
Inline + cleaning

Here is my missing symbol. :)
I will update the patch tomorrow.

[Darin Adler]

This "cleaning" seems not very good.

Removing the constructor that makes an IntRect from a FloatRect seems OK.

What is the rationale for making the intersects function inline? Does it make the code size smaller or speed some operation up? Did you measure the speedup?

It is not appropriate to put a function like IntRect::scale inside an #ifdef. This is complicating, not simplifying, the IntRect class and the platform directory for little gain. It's a sort of "layering violation" to have these sorts of ifdefs and we should include them only if there is a massive benefit to doing so.

"I am tired of people complaining of the size of Webkit" is not sufficient reason to make a change like this that has a negligible effect on the size of WebKit.

[Benjamin Poulain]

Created attachment 50172 [details]
Inline IntRect::intersects() and the constructor taking a FloatRect

(In reply to comment #5)
> What is the rationale for making the intersects function inline? Does it make
> the code size smaller or speed some operation up? Did you measure the speedup?

Make the code smaller I don't think so, but it speed up the rendering a bit on ARM.


> It is not appropriate to put a function like IntRect::scale inside an #ifdef.
> This is complicating, not simplifying, the IntRect class and the platform
> directory for little gain. It's a sort of "layering violation" to have these
> sorts of ifdefs and we should include them only if there is a massive benefit
> to doing so.
> 
> "I am tired of people complaining of the size of Webkit" is not sufficient
> reason to make a change like this that has a negligible effect on the size of
> WebKit.

Yep, totally, tiredness got me yesterday, that was a stupid change.
As I said, I am really only interested in the inlining. :)

[Darin Adler]

(In reply to comment #6)
> (In reply to comment #5)
> > What is the rationale for making the intersects function inline? Does it make
> > the code size smaller or speed some operation up? Did you measure the speedup?
> 
> Make the code smaller I don't think so, but it speed up the rendering a bit on
> ARM.

Would you be willing to share how much it sped up rendering?

[Benjamin Poulain]

(In reply to comment #7)
> > Make the code smaller I don't think so, but it speed up the rendering a bit on
> > ARM.
> 
> Would you be willing to share how much it sped up rendering?

Not much, it was <1%.
I can get the instructions count with valgrind. If you want the time difference, I should be able to do it on Monday with a N900.

[Darin Adler]

I would like to know. If it's measurable it does seem worthwhile to make the change.

By the way, I would prefer to do it by adding an inline modifier and moving the function to the header rather than putting the function definition inside the class definition.

It's good to make the class definition itself as readable as possible, and having long-ish function definitions inline in the class definition does seem to adversely affect readability.

[Benjamin Poulain]

Comment on attachment 50172 [details]
Inline IntRect::intersects() and the constructor taking a FloatRect

Only 0.01% improvement in Valgrind, not worth it.