Created attachment 319296 [details] Proof of concept (apply to r219089) We talked about adding a unified source build in the webkit-dev mailing list. This is the bug for the JSC transition.
Note that in the PoC I had to rename Air to not be under B3. I'm not sure that will be needed if we switch to per directory bundles.
Indeed, do or do not. There is no try.
BTW, as I probably said before, I propose to use existing WebCore's *AllInOne.cpp infrastructure with fixed AllInOne.cpp files, so that set of files and their order are detereministic
(In reply to Konstantin Tokarev from comment #3) > BTW, as I probably said before, I propose to use existing WebCore's > *AllInOne.cpp infrastructure with fixed AllInOne.cpp files, so that set of > files and their order are detereministic IIRC, when this was discussed on the webkit-dev mailing list the conclusion was to to do that. The downside of all-in-one cpp files is that we would need to have hundreds of all-in-one cpp files if we want to have ~8 files per bundle. The other alternative is to have a txt file with all the files in the bundle but at that point we might as well have the system do the load balancing for us.
Created attachment 320438 [details] Patch
Comment on attachment 320438 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320438&action=review > Source/JavaScriptCore/CMakeLists.txt:1020 > +set(_unifiedSourceCount 0) I mean, move PROCESS_ALLINONE_FILE from WebCore/WebCoreMacros.cmake to WebKitMacros and use here
Comment on attachment 320438 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320438&action=review >> Source/JavaScriptCore/CMakeLists.txt:1020 >> +set(_unifiedSourceCount 0) > > I mean, move PROCESS_ALLINONE_FILE from WebCore/WebCoreMacros.cmake to WebKitMacros and use here This is temporary code. I plan to move most of this logic to a separate script before landing so that we can use the same logic between Xcode and CMake.
Makes sense, parsing C++ files with cmake code never felt right to me
Created attachment 320442 [details] WIP
Attachment 320442 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/BinarySwitch.cpp:219: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 62 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 320459 [details] WIP
Attachment 320459 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/BinarySwitch.cpp:219: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 59 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 320487 [details] WIP
Attachment 320487 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/BinarySwitch.cpp:219: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 62 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 320524 [details] Patch
Attachment 320524 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/BinarySwitch.cpp:219: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 70 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 320552 [details] Patch
Attachment 320552 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/BinarySwitch.cpp:219: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 70 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 320559 [details] Patch
Attachment 320559 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/BinarySwitch.cpp:219: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 69 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 320560 [details] Patch
Attachment 320560 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/BinarySwitch.cpp:219: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 70 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 320559 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320559&action=review > Source/JavaScriptCore/ChangeLog:8 > + This patch switchs the CMake JavaScriptCore build to use unified sources. Typo: switchs => switches > Source/JavaScriptCore/ChangeLog:269 > + * CMakeLists.txt: Double Changelog. > Source/WTF/ChangeLog:10 > + important that we use the same script to generate the the bundles Grammar: "the the" => the > Source/JavaScriptCore/CMakeLists.txt:1021 > + if (NOT (${_sourceFile} MATCHES ".*c$")) How about using just /c$/ the ".*" seems unnecessary here. > Source/JavaScriptCore/CMakeLists.txt:1034 > + message("WARNING: unified-source-bundler.rb exited with non-zero status not appending results") I feel like this should trigger an error / failure. Seems weird that we then fall down to potentially succeed later. I thin you can do this with: message(FATAL_ERROR, "...") > Source/JavaScriptCore/CMakeLists.txt:1596 > +message(${JavaScriptCore_SOURCES}) This is debug?
Comment on attachment 320560 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320560&action=review I'd like to see another version of the ruby, since it should be much clearer after addressing comments. > Source/WTF/generate-unified-source-bundles.rb:1 > +# Copyright (C) 2011, 2016 Apple Inc. All rights reserved. Copyright date > Source/WTF/generate-unified-source-bundles.rb:28 > +def usage This should describe the expected usage, since apparently you use ARGV[0] later on but its not described puts "#{File.basename($0)} -p <path> [options] ..." > Source/WTF/generate-unified-source-bundles.rb:38 > +# Pick an unreasonable number because... whatever. This comment is unhelpful. Keep the fixme for the unused numbers (or remove them and add them when they are used). But when you do use them provide a descriptive comment explaining them. You also don't cover these variables in the `usage`. How about just adding them when they are actually used? > Source/WTF/generate-unified-source-bundles.rb:41 > +$maxCBundleCount = 100000 You said you can't / don't support C files. Drop this one? > Source/WTF/generate-unified-source-bundles.rb:84 > + def flush() Style: Parenthesis not needed here > Source/WTF/generate-unified-source-bundles.rb:87 > + if (@currentBundleFile) Style: Parenthesis are not needed > Source/WTF/generate-unified-source-bundles.rb:93 > + @currentBundleFile = $derivedSourcesPath + "UnifiedSource#{($generatedSources.length + 1).to_s + extension}" I suggest using a local variable for the filename for readability. Also probably using @bundleCount might be clearer than $generatedSources.length: nextUnifiedSourceFilename = "UnifiedSource#{@bundleCount}#{extension}" @currentBundleFile = $derivedSourcesPath + nextUnifiedSourceFilename > Source/WTF/generate-unified-source-bundles.rb:100 > + if (@fileCount % MAX_BUNDLE_SIZE == 0) Style: Parenthesis are not needed. Also, can we simplify this to just equality instead of mod arithmetic? if @fileCount == MAX_BUNDLE_SIZE This would require reworking how you call flush. Right now this is somewhat confusing. The first time you call flush it doesn't write anything and instead sets up variables. That is rather confusing. I'd expect `flush` to always write a UnifiedSource and increment if there are contents. > Source/WTF/generate-unified-source-bundles.rb:110 > +#FIXME: We should figure out how to bundle C files. The lack of namespaces makes this difficult... > +class CFileManager I suggest naming this: `NonBundleFileManager` and using it for ".c" and ".m" sources. How do you handle ".cc" files in WTF which are C++. > Source/WTF/generate-unified-source-bundles.rb:127 > +$currentDirectory = Pathname.new(ARGV[0]).dirname > + > +ARGV.sort.each { Seems weird to initialize $currentDirectory like this outside the loop. ARGV[0] might be nil. You should also validate ARGV.size is > 0 and dump usage if it is 0. You expect at least one source file. > Source/WTF/generate-unified-source-bundles.rb:150 > +$stdout.write($generatedSources.join(";") + ";") Style: Just use `print ...` instead of `$stdout.write(...)`
Comment on attachment 320560 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320560&action=review >> Source/WTF/generate-unified-source-bundles.rb:110 >> +class CFileManager > > I suggest naming this: `NonBundleFileManager` and using it for ".c" and ".m" sources. > > How do you handle ".cc" files in WTF which are C++. These .cc files are 3rd-party code (Google's double-conversion), so I'd argue we don't need to bundle them
Comment on attachment 320560 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320560&action=review >> Source/WTF/generate-unified-source-bundles.rb:1 >> +# Copyright (C) 2011, 2016 Apple Inc. All rights reserved. > > Copyright date Fixed. >> Source/WTF/generate-unified-source-bundles.rb:28 >> +def usage > > This should describe the expected usage, since apparently you use ARGV[0] later on but its not described > > puts "#{File.basename($0)} -p <path> [options] ..." Updated. >> Source/WTF/generate-unified-source-bundles.rb:38 >> +# Pick an unreasonable number because... whatever. > > This comment is unhelpful. Keep the fixme for the unused numbers (or remove them and add them when they are used). But when you do use them provide a descriptive comment explaining them. > > You also don't cover these variables in the `usage`. How about just adding them when they are actually used? I'm going to use them in my next patch so I think it's fine to have a little slop here. >> Source/WTF/generate-unified-source-bundles.rb:41 >> +$maxCBundleCount = 100000 > > You said you can't / don't support C files. Drop this one? Fixed. >> Source/WTF/generate-unified-source-bundles.rb:84 >> + def flush() > > Style: Parenthesis not needed here fixed. >> Source/WTF/generate-unified-source-bundles.rb:87 >> + if (@currentBundleFile) > > Style: Parenthesis are not needed fixed. >> Source/WTF/generate-unified-source-bundles.rb:93 >> + @currentBundleFile = $derivedSourcesPath + "UnifiedSource#{($generatedSources.length + 1).to_s + extension}" > > I suggest using a local variable for the filename for readability. Also probably using @bundleCount might be clearer than $generatedSources.length: > > nextUnifiedSourceFilename = "UnifiedSource#{@bundleCount}#{extension}" > @currentBundleFile = $derivedSourcesPath + nextUnifiedSourceFilename Done. I actually removed the class member because it's not used outside this function anyway. and I simplified this code. >> Source/WTF/generate-unified-source-bundles.rb:100 >> + if (@fileCount % MAX_BUNDLE_SIZE == 0) > > Style: Parenthesis are not needed. > > Also, can we simplify this to just equality instead of mod arithmetic? > > if @fileCount == MAX_BUNDLE_SIZE > > This would require reworking how you call flush. Right now this is somewhat confusing. The first time you call flush it doesn't write anything and instead sets up variables. That is rather confusing. I'd expect `flush` to always write a UnifiedSource and increment if there are contents. Done. >>> Source/WTF/generate-unified-source-bundles.rb:110 >>> +class CFileManager >> >> I suggest naming this: `NonBundleFileManager` and using it for ".c" and ".m" sources. >> >> How do you handle ".cc" files in WTF which are C++. > > These .cc files are 3rd-party code (Google's double-conversion), so I'd argue we don't need to bundle them We could also just rename them to .cpp files if we want to. >> Source/WTF/generate-unified-source-bundles.rb:127 >> +ARGV.sort.each { > > Seems weird to initialize $currentDirectory like this outside the loop. ARGV[0] might be nil. > > You should also validate ARGV.size is > 0 and dump usage if it is 0. You expect at least one source file. Done. >> Source/WTF/generate-unified-source-bundles.rb:150 >> +$stdout.write($generatedSources.join(";") + ";") > > Style: Just use `print ...` instead of `$stdout.write(...)` Fixed.
Created attachment 320569 [details] Patch
Attachment 320569 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/BinarySwitch.cpp:219: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 70 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 320569 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320569&action=review The ruby looks much better and more readable to me now. > Source/WTF/generate-unified-source-bundles.rb:49 > + ['--max-c-bundle-count', GetoptLong::REQUIRED_ARGUMENT], Remove > Source/WTF/generate-unified-source-bundles.rb:62 > + when '--max-c-bundle-count' > + $maxCBundleCount = arg Remove > Source/WTF/generate-unified-source-bundles.rb:68 > +usage if !$derivedSourcesPath || ARGV.length == 0 You could do: `ARGV.empty?` which is more natural Ruby than `ARGV.length == 0` > Source/WTF/generate-unified-source-bundles.rb:114 > +#FIXME: We should figure out how to bundle C files. The lack of namespaces makes this difficult... > +class CFileManager A better comment would be: "For files we don't know how to bundle, just pass them through". This doesn't need to be specific for ".c" files since it may apply to ".m" files as well when you get there. However, I think you can delete this code altogether! You could avoid creating bundle managers for ".c" and ".m" and instead... > Source/WTF/generate-unified-source-bundles.rb:143 > + raise "unknown source extension: #{path.extname} known extensions: #{$bundleManagers.keys.to_s}" unless bundle ... handle them here with: bundle = $bundleManagers[path.extname] if !bundle log("No bundle manger for #{path.extname}: #{path}") $generatedSources << file else bundle.addFile(path) > Source/WTF/generate-unified-source-bundles.rb:150 > +# Add trailing semicolon since CMake seems dislikes not having it. Grammar: "since CMake seems dislikes" > Source/WTF/generate-unified-source-bundles.rb:151 > +# Also, make sure we use print instead of puts because CMake will think the \n is a source file and fail to build. Don't say "use print instead of puts because ...", that is misleadingly specific. Instead just say "don't include a trailing newline because ...". > Source/WTF/generate-unified-source-bundles.rb:152 > +print($generatedSources.join(";") + ";") Style: No need for the parenthesis here: print $generatedSources.join(",") + ";"
(In reply to Build Bot from comment #28) > Attachment 320569 [details] did not pass style-queue: > > > ERROR: Source/JavaScriptCore/jit/BinarySwitch.cpp:219: Multi line control > clauses should use braces. [whitespace/braces] [4] > Total errors found: 1 in 70 files I wonder if this happens because the line of whitespace (line 219). I wonder if you remove that (or lessen it) if it gets rid of this errant style warning.
Comment on attachment 320569 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320569&action=review I think a good way to land this would be to land all the parts that make changes that make no effect, then land the rest. > Source/JavaScriptCore/CMakeLists.txt:49 > -set(JavaScriptCore_SOURCES > +set(JavaScriptCore_OG_SOURCES What does OG mean? I don't think we should use such an abbreviation. > Source/JavaScriptCore/CMakeLists.txt:1021 > + if (NOT (${_sourceFile} MATCHES "c$")) Do we want to check if it ends with .c or just if it ends with c? > Source/JavaScriptCore/CMakeLists.txt:1039 > +# These are special files that we can't or don't want to unified source compile Why not? Should we fix this?
Comment on attachment 320569 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320569&action=review >> Source/JavaScriptCore/CMakeLists.txt:1021 >> + if (NOT (${_sourceFile} MATCHES "c$")) > > Do we want to check if it ends with .c or just if it ends with c? I don't think it matters one way or another. I can make it .c if you'd prefer. >> Source/JavaScriptCore/CMakeLists.txt:1039 >> +# These are special files that we can't or don't want to unified source compile > > Why not? Should we fix this? Because it needs special compiler flags non-optimizing compiler flags that we don't want to disable elsewhere. >> Source/WTF/generate-unified-source-bundles.rb:49 >> + ['--max-c-bundle-count', GetoptLong::REQUIRED_ARGUMENT], > > Remove Whoops, Gone. >> Source/WTF/generate-unified-source-bundles.rb:62 >> + $maxCBundleCount = arg > > Remove Gone. >> Source/WTF/generate-unified-source-bundles.rb:68 >> +usage if !$derivedSourcesPath || ARGV.length == 0 > > You could do: `ARGV.empty?` which is more natural Ruby than `ARGV.length == 0` Oh, I didn't know that existed. >> Source/WTF/generate-unified-source-bundles.rb:114 >> +class CFileManager > > A better comment would be: "For files we don't know how to bundle, just pass them through". > This doesn't need to be specific for ".c" files since it may apply to ".m" files as well when you get there. > > However, I think you can delete this code altogether! You could avoid creating bundle managers for ".c" and ".m" and instead... Yeah, that seems like a better way to do it. >> Source/WTF/generate-unified-source-bundles.rb:152 >> +print($generatedSources.join(";") + ";") > > Style: No need for the parenthesis here: > > print $generatedSources.join(",") + ";" I kinda like calling it as a normal function.
(In reply to Alex Christensen from comment #31) > Comment on attachment 320569 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=320569&action=review > > I think a good way to land this would be to land all the parts that make > changes that make no effect, then land the rest. > I don't agree with this. I think it's better to just make sure all the changes make sense and land them. There's no point in trying to wait. That just makes it more likely that we'll have to keep updating the build to ensure it doesn't break between versions of the bundler.
Created attachment 320577 [details] Patch
Attachment 320577 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/BinarySwitch.cpp:219: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 70 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 320577 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320577&action=review r=me > Source/WTF/ChangeLog:21 > + up to 8 files from the same directory. We don't bundle files from Is "from the same directory" still true?
Comment on attachment 320577 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320577&action=review >> Source/WTF/ChangeLog:21 >> + up to 8 files from the same directory. We don't bundle files from > > Is "from the same directory" still true? Yeah, I kept it. I tried adding a few files and at was pretty annoying when everything shifts so you have to do a clean build.
Comment on attachment 320577 [details] Patch Clearing flags on attachment: 320577 Committed r221954: <http://trac.webkit.org/changeset/221954>
All reviewed patches have been landed. Closing bug.
Comment on attachment 320577 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320577&action=review > Source/JavaScriptCore/CMakeLists.txt:49 > -set(JavaScriptCore_SOURCES > +set(JavaScriptCore_OG_SOURCES I agree with Alex that the "OG" is cryptic and confusing at best. We should rename this to something meaningful and descriptive. Perhaps `JavaScriptCore_SOURCE_IN` to be in line with our `foo.in` files which is an indication it needs to be processed. JavaScriptCore_UNBUNDLED_SOURCES, JavaScriptCore_ALL_SOURCES, JavaScriptCore_x_SOURCES. At this point just about anything is better than OG.
Comment on attachment 320577 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320577&action=review > Source/WTF/ChangeLog:32 > + * generate-unified-source-bundles.rb: Added. I don't think this script should be under WTF, it should be under Tools/Scripts/webkitruby. Almost all of the scripts that are required by the build are already kept under Tools; no need for this to be an exception. >> Source/JavaScriptCore/CMakeLists.txt:49 >> +set(JavaScriptCore_OG_SOURCES > > I agree with Alex that the "OG" is cryptic and confusing at best. We should rename this to something meaningful and descriptive. > > Perhaps `JavaScriptCore_SOURCE_IN` to be in line with our `foo.in` files which is an indication it needs to be processed. > > JavaScriptCore_UNBUNDLED_SOURCES, JavaScriptCore_ALL_SOURCES, JavaScriptCore_x_SOURCES. > > At this point just about anything is better than OG. I haven't been following this bug closely, but I was going to complain about this too. I also have no clue what OG stands for. > Source/JavaScriptCore/CMakeLists.txt:1033 > + message(FATAL_ERROR "unified-source-bundler.rb exited with non-zero status not appending results") Is this missing a comma? Since the error is fatal, a better message would be simply "unified-source-bundler.rb exited with non-zero status"
(In reply to Michael Catanzaro from comment #41) > Comment on attachment 320577 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=320577&action=review > > > Source/WTF/ChangeLog:32 > > + * generate-unified-source-bundles.rb: Added. > > I don't think this script should be under WTF, it should be under > Tools/Scripts/webkitruby. Almost all of the scripts that are required by the > build are already kept under Tools; no need for this to be an exception. Due to Apple's internal build system rules, we need this script to be in one of the Source subdirectories. You'll notice that all the Xcode scripts are duplicated across directories. I'd like to use the same script for all the projects in WebKit that we will do unified sources with. Which, unfortunately, means that the script needs to be forwarded from the WTF. > > >> Source/JavaScriptCore/CMakeLists.txt:49 > >> +set(JavaScriptCore_OG_SOURCES > > > > I agree with Alex that the "OG" is cryptic and confusing at best. We should rename this to something meaningful and descriptive. > > > > Perhaps `JavaScriptCore_SOURCE_IN` to be in line with our `foo.in` files which is an indication it needs to be processed. > > > > JavaScriptCore_UNBUNDLED_SOURCES, JavaScriptCore_ALL_SOURCES, JavaScriptCore_x_SOURCES. > > > > At this point just about anything is better than OG. > > I haven't been following this bug closely, but I was going to complain about > this too. I also have no clue what OG stands for. Whoops, I meant to change this but I forgot... :/ > > > Source/JavaScriptCore/CMakeLists.txt:1033 > > + message(FATAL_ERROR "unified-source-bundler.rb exited with non-zero status not appending results") > > Is this missing a comma? Since the error is fatal, a better message would be > simply "unified-source-bundler.rb exited with non-zero status" If you mean a comma between "FATAL_ERROR" and "unified-source-bundler.rb...", no because CMake is weird. I agree that the "not appending results" part of the message is unnecessary.
I uploaded a follow-up patch: https://bugs.webkit.org/show_bug.cgi?id=176823
<rdar://problem/34694175>