Unified source builds: A new rule for static variables
Hello WebKittens, As many of you already know build times in WebKit have a major impact on the productivity of developers. My hope is to change that for clean builds without significantly impacting incremental builds. We’re expecting to see a 3-4x build time speedup on clean builds of WebKit! The current plan is to use unified sources to solve this problem. There’s one change you need to be aware of: static variables in .cpp files. When you want to declare a static function or variable (furiable? 🐈) in a .cpp file (I hope you’re not doing that in a header!) you’ll need to put that static in namespace FILENAME { }. For example: namespace FILENAME { static int myVariable { 0 }; static int myFunction() { return myVariable; } } We need to have the namespace because unified source builds compile multiple cpp files as if they were one cpp file so there could be name conflicts. Due to build magic, FILENAME will be the name of the cpp file you are compiling. If you want to know more about how this works you can check out my other email that goes into more detail. Cheers, Keith
How does this work? Without a “using” how does it know to search this namespace? Is this superior to using anonymous namespaces instead of “static”? — Darin
I indeed think this will require “using” statements or explicit namespace at call sites. I don’t think anonymous namespaces are suitable for resolving naming conflicts due to unity builds since the functions and up in the same compilation unit. -- Chris Dumez
On Aug 29, 2017, at 9:00 AM, Darin Adler <darin@apple.com> wrote:
How does this work? Without a “using” how does it know to search this namespace? Is this superior to using anonymous namespaces instead of “static”?
— Darin _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
On Aug 29, 2017, at 9:04 AM, Chris Dumez <cdumez@apple.com> wrote:
I indeed think this will require “using” statements or explicit namespace at call sites.
Yeah, this is basically what’s required. Unfortunately, if you ‘using namespace’ in a namespace all subsequent copies of that namespace will also see the ‘using’. e.g. namespace Foo { namespace Bar { int myVar { 0 }; } using namespace Bar; } namespace Foo { namespace Baz { int myVar { 0 }; } using namespace Baz; int myFunction() { return myVar; } } Will fail since myVar could be Baz::myVar or Bar::myVar. Using ‘using namespace’ inside a function or class body should be fine however.
I don’t think anonymous namespaces are suitable for resolving naming conflicts due to unity builds since the functions and up in the same compilation unit.
That’s right.
-- Chris Dumez
On Aug 29, 2017, at 9:00 AM, Darin Adler <darin@apple.com <mailto:darin@apple.com>> wrote:
How does this work? Without a “using” how does it know to search this namespace? Is this superior to using anonymous namespaces instead of “static”?
— Darin _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org <mailto:webkit-dev@lists.webkit.org> https://lists.webkit.org/mailman/listinfo/webkit-dev
On Aug 29, 2017, at 9:29 AM, Darin Adler <darin@apple.com> wrote:
On Aug 29, 2017, at 9:12 AM, Keith Miller <keith_miller@apple.com> wrote:
Unfortunately, if you ‘using namespace’ in a namespace all subsequent copies of that namespace will also see the ‘using’.
So this requires FILENAME:: at each call site?
Sadly, Yes. :(
— Darin
If we decide that we can’t support file scope identifiers then we should figure out the most practical way to do it. Of course this affects constants and variables, too, not just functions. I think this special FILENAME namespace isn’t all that helpful or needed. If a file contains a class like, say, Element, then we can name the namespace ElementInternals or whatever else seems logical. Calling it FILENAME instead doesn’t make things more readable nor more foolproof and is also likely to confuse development tools unnecessarily. Note that we can use “using namespace” inside functions if needed, but not at the file level. I think the harder part is how to enforce this rule if the theory is that we can’t avoid conflicts with ad hoc naming. Using namespaces isn’t fool proof unless there is something that checks for accidental use of the global scope. I don’t see that universal use of namespaces is required to avoid conflicts. We manage to keep things unique across the whole project in headers using a combination of namespaces and naming and I don’t see why it would require such a firm rule inside source files as long as we clarify the uniqueness requirement. So I think we should not do the FILENAME thing and we should maybe not even emphasize mandatory use of namespaces to avoid conflicts. Instead I suggest we focus on making sure we tools that help us detect conflicts before code is checked in even before we make the transition to this new way of building. — Darin
On Aug 29, 2017, at 10:15 AM, Darin Adler <darin@apple.com> wrote:
If we decide that we can’t support file scope identifiers then we should figure out the most practical way to do it. Of course this affects constants and variables, too, not just functions.
I think this special FILENAME namespace isn’t all that helpful or needed. If a file contains a class like, say, Element, then we can name the namespace ElementInternals or whatever else seems logical. Calling it FILENAME instead doesn’t make things more readable nor more foolproof and is also likely to confuse development tools unnecessarily. Note that we can use “using namespace” inside functions if needed, but not at the file level.
I’m not particularly opposed to using a different namespace name. Unfortunately, that still doesn’t help with the ElementInternal:: invocation problem since you can’t forward ‘using namespace ElementInternal’ inside the class declaration of Element.
I think the harder part is how to enforce this rule if the theory is that we can’t avoid conflicts with ad hoc naming. Using namespaces isn’t fool proof unless there is something that checks for accidental use of the global scope.
I agree that we will need have some tool that prevents things from being placed into the global scope. My plan was to do that after getting the build working, with the belief that the issue would be relatively contained for the brief period between when we get unified builds working and the linter is place. This gets the most value to developers as soon as possible, which seems very valuable.
I don’t see that universal use of namespaces is required to avoid conflicts. We manage to keep things unique across the whole project in headers using a combination of namespaces and naming and I don’t see why it would require such a firm rule inside source files as long as we clarify the uniqueness requirement.
To be clear, I don’t think it’s required. Many other projects use unified source builds without using namespaces to solve the problem. My primary reasoning for using namespaces is a combination of avoiding the mental cost of thinking about the name collisions and just applying a simple rule. I’d also like to make this easier to understand for WebKit noobies, which is why I thought FILENAME would be simpler to wrap their heads around.
So I think we should not do the FILENAME thing and we should maybe not even emphasize mandatory use of namespaces to avoid conflicts. Instead I suggest we focus on making sure we tools that help us detect conflicts before code is checked in even before we make the transition to this new way of building.
Without the namespace rule, any tool that checks for global namespace collisions won’t be all that useful for regular development. I doubt anyone is going to run such a script before they go to upload a patch to bugzilla. Even if they did it’s not clear that it would help them any more than just trying to compile and fixing the issue after they see the build error. So developers will still hit the name collision issue randomly throughout development.
— Darin
Sent from my iPhone
On Aug 29, 2017, at 11:22 AM, Keith Miller <keith_miller@apple.com> wrote:
I doubt anyone is going to run such a script before they go to upload a patch to bugzilla.
EWS was what I was hoping for; likely to be sufficient. But it could also be integrated into the development process as, say, check-webkit-style is.
So developers will still hit the name collision issue randomly throughout development.
Sure. But I don’t think that required extensive use of namespaces is the best way to greatly mitigate this. Mistakes will still happen. So I think we shouldn’t go too far in ruining readability of code for something that is not necessary to solve the problem. Recommending either namespaces or globally unique names and clarifying that file local scope doesn’t exist are both good. But again I think people already handle these problems fine in headers so we don’t need too tight a straitjacket, at least not out of the gate. — Darin
On Aug 29, 2017, at 11:31 AM, Darin Adler <darin@apple.com> wrote:
Sent from my iPhone
On Aug 29, 2017, at 11:22 AM, Keith Miller <keith_miller@apple.com> wrote:
I doubt anyone is going to run such a script before they go to upload a patch to bugzilla.
EWS was what I was hoping for; likely to be sufficient. But it could also be integrated into the development process as, say, check-webkit-style is.
check-webkit-style is run by both EWS and webkit-patch upload, in addition to being hand-runnable, so that seems like a good place to put this new kind of check.
So developers will still hit the name collision issue randomly throughout development.
Sure.
But I don’t think that required extensive use of namespaces is the best way to greatly mitigate this. Mistakes will still happen. So I think we shouldn’t go too far in ruining readability of code for something that is not necessary to solve the problem.
Recommending either namespaces or globally unique names and clarifying that file local scope doesn’t exist are both good.
But again I think people already handle these problems fine in headers so we don’t need too tight a straitjacket, at least not out of the gate.
I tend to agree with this. I think keeping names of static functions globally unique is reasonable, so long as we have an automated way to check. This seems better than namespaces. With namespaces, it's still possible to make a mistake, such as by having a using at global scope, so we'd need the style checker to enforce some kind of rule. If we were to use namespaces, then properly naming them seems better than the FILENAME macro. Regards, Maciej
On Tue, Aug 29, 2017 at 1:37 PM, Maciej Stachowiak <mjs@apple.com> wrote:
I tend to agree with this. I think keeping names of static functions globally unique is reasonable, so long as we have an automated way to check. This seems better than namespaces. With namespaces, it's still possible to make a mistake, such as by having a using at global scope, so we'd need the style checker to enforce some kind of rule.
If we were to use namespaces, then properly naming them seems better than the FILENAME macro.
Agreed. I posted a somewhat different proposal in the other thread, suggesting that we manually decide which files to unify together so that we only need to keep names of static functions unique to a particular unified source bundle. That would be easier than keeping the names globally-unique, but would complicate the build system. Either way would be fine I think: I'm really just hoping to avoid the need to adopt this namespace FILENAME rule. :) Michael
On Aug 29, 2017, at 11:37 AM, Maciej Stachowiak <mjs@apple.com> wrote:
On Aug 29, 2017, at 11:31 AM, Darin Adler <darin@apple.com> wrote:
Sent from my iPhone
On Aug 29, 2017, at 11:22 AM, Keith Miller <keith_miller@apple.com> wrote:
I doubt anyone is going to run such a script before they go to upload a patch to bugzilla.
EWS was what I was hoping for; likely to be sufficient. But it could also be integrated into the development process as, say, check-webkit-style is.
check-webkit-style is run by both EWS and webkit-patch upload, in addition to being hand-runnable, so that seems like a good place to put this new kind of check.
I agree and my intention was to add the check to check-webkit-style.
So developers will still hit the name collision issue randomly throughout development.
Sure.
But I don’t think that required extensive use of namespaces is the best way to greatly mitigate this. Mistakes will still happen. So I think we shouldn’t go too far in ruining readability of code for something that is not necessary to solve the problem.
Recommending either namespaces or globally unique names and clarifying that file local scope doesn’t exist are both good.
But again I think people already handle these problems fine in headers so we don’t need too tight a straitjacket, at least not out of the gate.
I tend to agree with this. I think keeping names of static functions globally unique is reasonable, so long as we have an automated way to check. This seems better than namespaces. With namespaces, it's still possible to make a mistake, such as by having a using at global scope, so we'd need the style checker to enforce some kind of rule.
If we were to use namespaces, then properly naming them seems better than the FILENAME macro.
I’ll defer to your judgement on properly naming the namespaces over using the macro. Does anyone have strong opinions on what rules the names should follow? I think Darin suggested <Filename>Internal. I think I would prefer <Filename>Static as that’s very clear what the namespace represents. I think I have been convinced that, for the most part we shouldn’t need to have such a strict rule on naming collisions. There are probably places where it makes more sense to have the namespace and places where it doesn’t. Here’s my proposal for the style checker. It should require that there are no duplicate globally visible variables in a given directory. We don’t need to worry about different directories since those files can’t be included in the same bundle under my proposal. While, it could be inconvenient for new developers it should keep the code much cleaner. Additionally, we can have a bot that builds with full per directory includes to ensure that we don’t have any collisions. As an aside, we might also want a bot that builds without unified sources to ensure people don’t rely on headers that happen to be above them in the bundle. Does this seem reasonable?
Regards, Maciej
On Tue, Aug 29, 2017 at 2:14 PM, Keith Miller <keith_miller@apple.com> wrote:
On Aug 29, 2017, at 11:37 AM, Maciej Stachowiak <mjs@apple.com> wrote:
On Aug 29, 2017, at 11:31 AM, Darin Adler <darin@apple.com> wrote:
Sent from my iPhone
On Aug 29, 2017, at 11:22 AM, Keith Miller <keith_miller@apple.com> wrote:
I doubt anyone is going to run such a script before they go to upload a patch to bugzilla.
EWS was what I was hoping for; likely to be sufficient. But it could also be integrated into the development process as, say, check-webkit-style is.
check-webkit-style is run by both EWS and webkit-patch upload, in addition to being hand-runnable, so that seems like a good place to put this new kind of check.
I agree and my intention was to add the check to check-webkit-style.
So developers will still hit the name collision issue randomly throughout development.
Sure.
But I don’t think that required extensive use of namespaces is the best way to greatly mitigate this. Mistakes will still happen. So I think we shouldn’t go too far in ruining readability of code for something that is not necessary to solve the problem.
Recommending either namespaces or globally unique names and clarifying that file local scope doesn’t exist are both good.
But again I think people already handle these problems fine in headers so we don’t need too tight a straitjacket, at least not out of the gate.
I tend to agree with this. I think keeping names of static functions globally unique is reasonable, so long as we have an automated way to check. This seems better than namespaces. With namespaces, it's still possible to make a mistake, such as by having a using at global scope, so we'd need the style checker to enforce some kind of rule.
If we were to use namespaces, then properly naming them seems better than the FILENAME macro.
I’ll defer to your judgement on properly naming the namespaces over using the macro. Does anyone have strong opinions on what rules the names should follow? I think Darin suggested <Filename>Internal. I think I would prefer <Filename>Static as that’s very clear what the namespace represents. I think I have been convinced that, for the most part we shouldn’t need to have such a strict rule on naming collisions. There are probably places where it makes more sense to have the namespace and places where it doesn’t.
As Darin suggested, <Class>Internal is appropriate for files like Element.cpp, Node.cpp, etc... which contains definitions of a class. There's an existing convention in WebKit that implementation details are suffixed by "Internal". It's probably more appropriate to use <Filename>Static for files like Editing.cpp, which is a collection of a bunch of global functions.
Here’s my proposal for the style checker. It should require that there are no duplicate globally visible variables in a given directory. We don’t need to worry about different directories since those files can’t be included in the same bundle under my proposal. While, it could be inconvenient for new developers it should keep the code much cleaner. Additionally, we can have a bot that builds with full per directory includes to ensure that we don’t have any collisions. As an aside, we might also want a bot that builds without unified sources to ensure people don’t rely on headers that happen to be above them in the bundle.
Makes sense. - R. Niwa
On Aug 29, 2017, at 4:32 PM, Ryosuke Niwa <rniwa@webkit.org> wrote:
It's probably more appropriate to use <Filename>Static for files like Editing.cpp, which is a collection of a bunch of global functions.
The keyword “static” is being used to ask for what C++ calls “internal linkage” <http://en.cppreference.com/w/cpp/language/storage_duration <http://en.cppreference.com/w/cpp/language/storage_duration>>. So we could even consider “internal” in those cases, unless we can come up with even better words. — Darin
In addition to what Darin said, I think using a consistent name makes the most sense. If “<Filename>Internal” is what we would use for some of the places we should use it everywhere. Cheers, Keith
On Aug 29, 2017, at 5:29 PM, Darin Adler <darin@apple.com> wrote:
On Aug 29, 2017, at 4:32 PM, Ryosuke Niwa <rniwa@webkit.org <mailto:rniwa@webkit.org>> wrote:
It's probably more appropriate to use <Filename>Static for files like Editing.cpp, which is a collection of a bunch of global functions.
The keyword “static” is being used to ask for what C++ calls “internal linkage” <http://en.cppreference.com/w/cpp/language/storage_duration <http://en.cppreference.com/w/cpp/language/storage_duration>>.
So we could even consider “internal” in those cases, unless we can come up with even better words.
— Darin
participants (6)
-
Chris Dumez
-
Darin Adler
-
Keith Miller
-
Maciej Stachowiak
-
Michael Catanzaro
-
Ryosuke Niwa