Bug 115556 - [CMake] Don't compile SVG/MathML files if they are not enabled
Summary: [CMake] Don't compile SVG/MathML files if they are not enabled
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-03 10:45 PDT by Mark Salisbury
Modified: 2016-06-27 23:42 PDT (History)
11 users (show)

See Also:


Attachments
Proposed fix (21.13 KB, patch)
2013-05-03 10:52 PDT, Mark Salisbury
no flags Details | Formatted Diff | Diff
Proposed fix (21.13 KB, patch)
2013-05-03 10:59 PDT, Mark Salisbury
rwlbuis: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (525.14 KB, application/zip)
2013-05-04 15:12 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Salisbury 2013-05-03 10:45:48 PDT
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.
Comment 1 Mark Salisbury 2013-05-03 10:52:50 PDT
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.
Comment 2 WebKit Commit Bot 2013-05-03 10:56:05 PDT
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.
Comment 3 Mark Salisbury 2013-05-03 10:59:29 PDT
Created attachment 200440 [details]
Proposed fix

Removed space between list and parenthesis.  Should pass style bot now.
Comment 4 Ryuan Choi 2013-05-03 21:23:55 PDT
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.
Comment 5 Ryuan Choi 2013-05-03 21:25:48 PDT
(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?
Comment 6 Gyuyoung Kim 2013-05-03 23:30:59 PDT
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.
Comment 7 Build Bot 2013-05-04 15:12:29 PDT
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
Comment 8 Build Bot 2013-05-04 15:12:32 PDT
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
Comment 9 Mark Salisbury 2013-05-06 15:03:29 PDT
(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.
Comment 10 Mark Salisbury 2013-05-06 16:42:18 PDT
(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.
Comment 11 Gyuyoung Kim 2013-05-07 02:04:27 PDT
(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 ?
Comment 12 Mark Salisbury 2013-05-07 08:16:00 PDT
(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.
Comment 13 Rob Buis 2013-05-09 12:14:54 PDT
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.
Comment 14 Mark Salisbury 2013-05-09 12:19:43 PDT
(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>".
Comment 15 Gyuyoung Kim 2013-05-09 18:54:46 PDT
(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.
Comment 16 Ryuan Choi 2013-05-09 19:07:56 PDT
(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?)
Comment 17 Gyuyoung Kim 2013-05-09 20:15:26 PDT
(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.
Comment 18 Rob Buis 2013-05-10 11:45:17 PDT
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 :)