Bug 115556 – [CMake] Don't compile SVG/MathML files if they are not enabled

• Status: UNCONFIRMED
• Component: WebCore Misc.

[Mark Salisbury]

If you are building with CMake and don't have SVG/MATHML enabled, there's no need to add these files to the source list.  It just slows down the build.

[Mark Salisbury]

Created attachment 200439 [details]
Proposed fix

This addresses the title of the bug.  In addition, style sheets for SVG and MATHML are not included if the features are not enabled.

[WebKit Commit Bot]

Attachment 200439 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog']" exit_code: 1
Source/WebCore/CMakeLists.txt:2302:  No space between command "list" and its parentheses, should be "list("  [whitespace/parentheses] [5]
Source/WebCore/CMakeLists.txt:2322:  No space between command "list" and its parentheses, should be "list("  [whitespace/parentheses] [5]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Salisbury]

Created attachment 200440 [details]
Proposed fix

Removed space between list and parenthesis.  Should pass style bot now.

[Ryuan Choi]

Comment on attachment 200440 [details]
Proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=200440&action=review

> Source/WebCore/ChangeLog:6
> +        Unreviewed.

This patch is back to previous. At least, I think that it should be reviewed.

[Ryuan Choi]

(In reply to comment #0)
> If you are building with CMake and don't have SVG/MATHML enabled, there's no need to add these files to the source list.  It just slows down the build.

Did you check how much time was decreased with this patch?

[Gyuyoung Kim]

SVG and MathML files are guarded using ENABLE() macro by itself. Isn't this overlapping guard ? As far as I know, GNUMakefile doesn't use ENABLE() guard for the files as well. If we maintain files using ENABLE() in CMake, same file sometimes was added to it. However, there was no big opinion not to use ENABLE() in CMake. If there is big time difference when using ENABLE(), I think we can use it. If not, I prefer to keep it as it is.

[Build Bot]

Comment on attachment 200440 [details]
Proposed fix

Attachment 200440 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/391881

New failing tests:
fast/frames/crash-remove-iframe-during-object-beforeload.html

[Build Bot]

Created attachment 200546 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[Mark Salisbury]

(In reply to comment #5)
> (In reply to comment #0)
> > If you are building with CMake and don't have SVG/MATHML enabled, there's no need to add these files to the source list.  It just slows down the build.
> 
> Did you check how much time was decreased with this patch?

With files -
  Build Time 37:21

Without -
  Build Time 34:16

This is just the time to build WebCore.lib (VS 2008, HP EliteBook 8560w with 8GB RAM).

It's almost a 10% savings in time, which isn't too surprising since I noticed webkit dropped by about 10% in size when I disabled SVG/MATHML.

WebKit.dll link time dropped from 57 to 55 seconds.

[Mark Salisbury]

(In reply to comment #6)
> SVG and MathML files are guarded using ENABLE() macro by itself. Isn't this overlapping guard ? As far as I know, GNUMakefile doesn't use ENABLE() guard for the files as well. If we maintain files using ENABLE() in CMake, same file sometimes was added to it. However, there was no big opinion not to use ENABLE() in CMake. If there is big time difference when using ENABLE(), I think we can use it. If not, I prefer to keep it as it is.

It is an overlapping guard.  If we had a single build system (CMake), we could take my patch and then remove the #if ENABLE(SVG) guards in the SVG files.

However, we don't have a single build system.  Even if we did, I don't mind the #if ENABLE(SVG) in the source files (in addition to putting if(ENABLE_SVG) in cmake files).  I think they make them more readable and tell you that SVG has to be turned on or the code is ignored.  They tell someone who isn't familiar with SVG that it's safe NOT to compile the file when SVG is not enabled.

I measured the build savings at 3 minutes, that's not trivial.

[Gyuyoung Kim]

(In reply to comment #9)
> (In reply to comment #5)
> > (In reply to comment #0)
> > > If you are building with CMake and don't have SVG/MATHML enabled, there's no need to add these files to the source list.  It just slows down the build.
> > 
> > Did you check how much time was decreased with this patch?
> 
> With files -
>   Build Time 37:21
> 
> Without -
>   Build Time 34:16
> 
> This is just the time to build WebCore.lib (VS 2008, HP EliteBook 8560w with 8GB RAM).

Interesting. There was no time saving on EFL port. Did you build it on windows ? What port did you built ? Was there no object caching like ccache ?

[Mark Salisbury]

(In reply to comment #11)
> (In reply to comment #9)
> > (In reply to comment #5)
> > > (In reply to comment #0)
> > > > If you are building with CMake and don't have SVG/MATHML enabled, there's no need to add these files to the source list.  It just slows down the build.
> > > 
> > > Did you check how much time was decreased with this patch?
> > 
> > With files -
> >   Build Time 37:21
> > 
> > Without -
> >   Build Time 34:16
> > 
> > This is just the time to build WebCore.lib (VS 2008, HP EliteBook 8560w with 8GB RAM).
> 
> Interesting. There was no time saving on EFL port. Did you build it on windows ? What port did you built ? Was there no object caching like ccache ?

I built WinCairo (I'm working on patches to make WinCairo a CMake port...).  I do not have ccache - that appears to work only with GCC.

[Rob Buis]

Comment on attachment 200440 [details]
Proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=200440&action=review

Mark, can you fix the Reviewed by line?

>> Source/WebCore/ChangeLog:6
>> +        Unreviewed.
> 
> This patch is back to previous. At least, I think that it should be reviewed.

This patch seems fine but I agree, please keep the normal Reviewed by line in.

[Mark Salisbury]

(In reply to comment #13)
> (From update of attachment 200440 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200440&action=review
> 
> Mark, can you fix the Reviewed by line?
> 
> >> Source/WebCore/ChangeLog:6
> >> +        Unreviewed.
> > 
> > This patch is back to previous. At least, I think that it should be reviewed.
> 
> This patch seems fine but I agree, please keep the normal Reviewed by line in.

Thanks Rob.

What should it be?

Reviewed by Rob Buis ?

When I submit a patch, changelog comments are required, which require a "reviewed by" statement, but at that point no one has reviewed the patch.  Sort of a chicken and egg problem...

"Unreviewed" passes the style bot, as well as "Reviewed by <reviewer>".

[Gyuyoung Kim]

(In reply to comment #14)
 
> When I submit a patch, changelog comments are required, which require a "reviewed by" statement, but at that point no one has reviewed the patch.  Sort of a chicken and egg problem...
> 
> "Unreviewed" passes the style bot, as well as "Reviewed by <reviewer>".

Please use "Tools/Scripts/prepare-ChangeLog <directory>". By the way, I still wonder why there is build time difference. All files are guarded by ENABLE(). Let me check this on EFL port again.

[Ryuan Choi]

(In reply to comment #15)
> (In reply to comment #14)
> 
> > When I submit a patch, changelog comments are required, which require a "reviewed by" statement, but at that point no one has reviewed the patch.  Sort of a chicken and egg problem...
> > 
> > "Unreviewed" passes the style bot, as well as "Reviewed by <reviewer>".
> 
> Please use "Tools/Scripts/prepare-ChangeLog <directory>". By the way, I still wonder why there is build time difference. All files are guarded by ENABLE(). Let me check this on EFL port again.

I double checked and it was fine in EFL port on linux.
(GCC 4.7.3 witout ccache on ubuntu 13.04 (64bit))

Well, I think that it should improve build performance because gcc does not need to look SVG stuffs.
I am not sure why my GCC(w/o ccache) had small improvement.(Or my GCC is well optimized for empty sourc file?)

[Gyuyoung Kim]

(In reply to comment #16)

> I am not sure why my GCC(w/o ccache) had small improvement.(Or my GCC is well optimized for empty sourc file?)

In EFL port case, 25 sec is more faster than existing one. IIRC, there was trivial time difference before. It looks we need to check why build time take a long time. Anyway, I also think 3 min is not trivial on wincairo. So, I don't mind to use guards in cmake again based on this build time measurement.

[Rob Buis]

Hi Mark,

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 200440 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=200440&action=review
> > 
> > Mark, can you fix the Reviewed by line?
> > 
> > >> Source/WebCore/ChangeLog:6
> > >> +        Unreviewed.
> > > 
> > > This patch is back to previous. At least, I think that it should be reviewed.
> > 
> > This patch seems fine but I agree, please keep the normal Reviewed by line in.
> 
> Thanks Rob.
> 
> What should it be?
> 
> Reviewed by Rob Buis ?
> 
> When I submit a patch, changelog comments are required, which require a "reviewed by" statement, but at that point no one has reviewed the patch.  Sort of a chicken and egg problem...
> 
> "Unreviewed" passes the style bot, as well as "Reviewed by <reviewer>".

I understand now. I agree with Gyuyoung Kim, using Tools/Scripts/prepare-ChangeLog <directory> prevents this problem. It should up showing like "Reviewed by (OOPS!)". It is better not to fill in the (who you think will be) reviewer name, that is a bit cheeky :)

[Ahmad Saleem]

I think we don't have SVG flag and also both SVG and MathML are shipping since long in Webkit. Do we need to do this anymore?

@Alexey - any idea who would be right person for this to close or comment on?

[Alexey Proskuryakov]

I see that we still have ENABLE_MATHML in the source, but not ENABLE_SVG. Seems like mostly something for Igalia and Sony to decide, as this is about CMake.