Re: [webkit-dev] [webkit-changes] [264332] trunk/Source

[Michael Catanzaro]

On Thu, Jul 16, 2020 at 4:14 pm, Darin Adler  wrote:
Let’s stop doing it that way. Just compile the files and have #if 
inside the file.


I removed all conditional source files from the Xcode build system. 
Let's do the same for the CMake build system.


— Darin


I agree we should do this. We've never been consistent between whether 
we guard files at the build system level or inside the files 
themselves. But with unified builds, the advantage of putting the 
guards inside the files is clear: we would ensure that the set of files 
compiled is always the same for a given port regardless of which 
options are used.


I wouldn't use that as a reason to drop non-unified builds, though. 
E.g. Yusuke's argument regarding compile_commands.json seems pretty 
compelling.



___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] [webkit-changes] [264332] trunk/Source

[Darin Adler]

> On Jul 16, 2020, at 3:33 PM, Adrian Perez de Castro  wrote:
> 
> enabling/disabling features changes the set of source files to
> build, which results in different source files being #included from each
> unifier source compilation unit.

Let’s stop doing it that way. Just compile the files and have #if inside the 
file.

I removed all conditional source files from the Xcode build system. Let's do 
the same for the CMake build system.

— Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] [webkit-changes] [264332] trunk/Source

[Adrian Perez de Castro]

On Thu, 16 Jul 2020 15:01:14 -0700, Darin Adler  wrote:
> > On Jul 16, 2020, at 2:28 PM, Adrian Perez de Castro  
> > wrote:
> > 
> > It is not feasible to test unified builds for all the possible combinations 
> > of
> > target architectures and set of features enabled at build time (there is a
> > combinatorial explosion of configurations). Keeping non-unified builds in a
> > working state guarantees that regular unified builds will keep working no
> > matter what the build configuration is.
> 
> But it doesn’t.

Yes, because enabling/disabling features changes the set of source files to
build, which results in different source files being #included from each
unifier source compilation unit.

> Non-unified builds don’t make it possible to have arbitrary sets of features
> enabled at build time. We break that all the time and it’s only fixed when
> someone tests that arbitrary set of features. Headers are only a small part
> of that issue.

True, and that is why our message for the GTK and WPE ports is always that
we only support builds with the default build options, but of course anyone
is free to change them their mileage may vary.

In practice, we tend to keep things working in a best-effort basis for the
following few cases:

 - Disabling the Wayland or X11 support in the GTK port.
 - Disabling the WPE renderer in the GTK port.
 - Disabling multimedia support in the WPE port.

> I’d go as far as saying it’s not a project goal to always build with
> arbitrary sets of features enabled.
 
I agree. But even toggling the few features I mentioned above, or the target
architecture to something else than what the bots use, results in build
failures related to unified builds due to different sources being #included
in each unified compilation unit ¯\_(ツ)_/¯ 

Cheers,
—Adrián


signature.asc
Description: PGP signature
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] [webkit-changes] [264332] trunk/Source

[Adrian Perez de Castro]

Hi,

On Fri, 17 Jul 2020 06:52:54 +0900, Fujii Hironori  
wrote:
> I'm glad to hear various opinions. Slack still can't beat mailing lists for
> technical discussions.

Aye, in my experience mailing lists are better because having to write down
thoughts in a slightly more structured way (in a mail message to be read
from start to end in one go) results in people organizing their thoughts
better (as opposed to a more or less random exchange of loosely organized
chat messages).

Not to mention that using a mailing list *also* records discussions and the
decisions, thanks to the messages being archived, which is more “durable” for
future reference. Oh, and everybody can easily grab a copy of the mailing list
archives (from the Mailman web interface as mboxes, or keeping their local
copy) so it's less likely that all copies of mailing list archives would
suddenly disappear; but OTOH, as unlikely as it may seem, Slack could go
bankrupt and/or the chat history vanish from one day to another.

> On Fri, Jul 17, 2020 at 6:37 AM Adrian Perez de Castro 
> wrote:
> 
> > Also, some packagers used to carry assorted downstream patches for build
> > issues related to unification build which have not been needed anymore.
> >
> 
> Unified source builds can have performance merit.
> The SQLite Amalgamation makes it 5% and 10% faster.
> https://www.sqlite.org/amalgamation.html

Well, this is what Link-Time Optimization is for, without needing to resort to
ugly preprocessor tricks. I would rather let the compiler and linker do their
job—it's 2020 and any serious toolchain supports it by now ;-)

Cheers,
—Adrián


signature.asc
Description: PGP signature
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] [webkit-changes] [264332] trunk/Source

[Darin Adler]

> On Jul 16, 2020, at 2:28 PM, Adrian Perez de Castro  wrote:
> 
> It is not feasible to test unified builds for all the possible combinations of
> target architectures and set of features enabled at build time (there is a
> combinatorial explosion of configurations). Keeping non-unified builds in a
> working state guarantees that regular unified builds will keep working no
> matter what the build configuration is.

But it doesn’t.

Non-unified builds don’t make it possible to have arbitrary sets of features 
enabled at build time. We break that all the time and it’s only fixed when 
someone tests that arbitrary set of features. Headers are only a small part of 
that issue.

I’d go as far as saying it’s not a project goal to always build with arbitrary 
sets of features enabled.

— Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] [webkit-changes] [264332] trunk/Source

[Fujii Hironori]

I'm glad to hear various opinions. Slack still can't beat mailing lists for
technical discussions.

On Fri, Jul 17, 2020 at 6:37 AM Adrian Perez de Castro 
wrote:

> Also, some packagers used to carry assorted downstream patches for build
> issues related to unification build which have not been needed anymore.
>

Unified source builds can have performance merit.
The SQLite Amalgamation makes it 5% and 10% faster.
https://www.sqlite.org/amalgamation.html
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] [webkit-changes] [264332] trunk/Source

[Adrian Perez de Castro]

I forgot to mention one tidbit...

On Fri, 17 Jul 2020 00:28:53 +0300, Adrian Perez de Castro  
wrote:
> Hello everybody,
> 
> On Thu, 16 Jul 2020 12:57:01 -0700, Darin Adler  wrote:
> > > On Jul 16, 2020, at 12:54 PM, Kirsling, Ross  
> > > wrote:
> > > 
> > > Non-unified build fixes are done to support *all* platforms, because
> > > sudden unification shifts can happen anywhere.
> >
> > I’m not sure that benefit is worth the additional code churn. I understand
> > that it’s frustrating to run into a problem when unification shifts, say
> > when you are adding a source file. I believe we have made changes to
> > unification since the earliest version that reduce how often unification
> > shifts.
>  
> While this is true, shifts still happen easily when changing anything in the
> build configuration that affects the list of source files to build. The cases
> in which I have seen this happening when using the CMake based ports are:
> 
> * The target architecture is not one of the few tested by the bots.
> 
>   - For the GTK and WPE ports this means that a build for anything else
> than x86_64 is susceptible to break easily.
> 
>   - Note that JSCOnly builds on ARM, MIPS, and i386 are actively tested by
> EWS bots, so the JSC build is less likely to.
> 
> * The features enabled at build time are changed from what the EWS bots test.
> 
>   - Happens when the build configuration (via CMake) changes the defaults.
> For example one can build the GTK port with support for Wayland enabled
>   and X11 disabled (the default is both enabled). Or one can build WPE 
> with
>   all media support disabled (which completely avoids needing GStreamer,
>   making the disk footprint considerably smaller: this is sometimes done
>   when targeting embedded devices).

As a matter of fact: lately I have been trying to make sure that non-unified
builds are working *right before* making source tarball releases for the WPE
port, and the reports of build failures due to unification issues has been
zero ever since.

Also, some packagers used to carry assorted downstream patches for build
issues related to unification build which have not been needed anymore. The
case I know better is Buildroot [1]: when I started maintaining its package
for the GTK port in preparation to submit the WPE package one of the tasks
I did was precisely avoid downstream patches, by applying the fixes to WebKit
itself—and *that* was when I learnt that making non-unified builds work was
a good thing.

>   - Bots test with experimental features enabled, but the default setting when
>   building from source tarballs has those disabled at build time. This is
>   less likely to result in a broken build than manually toggling features,
>   it can happen.
> 
> It is not feasible to test unified builds for all the possible combinations of
> target architectures and set of features enabled at build time (there is a
> combinatorial explosion of configurations). Keeping non-unified builds in a
> working state guarantees that regular unified builds will keep working no
> matter what the build configuration is.
> 
> Another side effect of keeping non-unified builds usable is that it is
> possible to build WebKit on machines with less RAM (granted: only release
> builds without any debugging information), which sometimes has enabled people
> who don't have access to beefier machines to get WebKit built and even work
> on patches. While this has not been very common, I think it's a commendable
> goal to allow people without access to beefy computers to contribute to
> WebKit :)
> 
> Best regards,
> —Adrián

---
[1] https://buildroot.org/


signature.asc
Description: PGP signature
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] [webkit-changes] [264332] trunk/Source

[Adrian Perez de Castro]

Hello everybody,

On Thu, 16 Jul 2020 12:57:01 -0700, Darin Adler  wrote:
> > On Jul 16, 2020, at 12:54 PM, Kirsling, Ross  wrote:
> > 
> > Non-unified build fixes are done to support *all* platforms, because
> > sudden unification shifts can happen anywhere.
>
> I’m not sure that benefit is worth the additional code churn. I understand
> that it’s frustrating to run into a problem when unification shifts, say
> when you are adding a source file. I believe we have made changes to
> unification since the earliest version that reduce how often unification
> shifts.
 
While this is true, shifts still happen easily when changing anything in the
build configuration that affects the list of source files to build. The cases
in which I have seen this happening when using the CMake based ports are:

* The target architecture is not one of the few tested by the bots.

  - For the GTK and WPE ports this means that a build for anything else
than x86_64 is susceptible to break easily.

  - Note that JSCOnly builds on ARM, MIPS, and i386 are actively tested by
EWS bots, so the JSC build is less likely to.

* The features enabled at build time are changed from what the EWS bots test.

  - Happens when the build configuration (via CMake) changes the defaults.
For example one can build the GTK port with support for Wayland enabled
and X11 disabled (the default is both enabled). Or one can build WPE 
with
all media support disabled (which completely avoids needing GStreamer,
making the disk footprint considerably smaller: this is sometimes done
when targeting embedded devices).

  - Bots test with experimental features enabled, but the default setting when
building from source tarballs has those disabled at build time. This is
less likely to result in a broken build than manually toggling features,
it can happen.

It is not feasible to test unified builds for all the possible combinations of
target architectures and set of features enabled at build time (there is a
combinatorial explosion of configurations). Keeping non-unified builds in a
working state guarantees that regular unified builds will keep working no
matter what the build configuration is.

Another side effect of keeping non-unified builds usable is that it is
possible to build WebKit on machines with less RAM (granted: only release
builds without any debugging information), which sometimes has enabled people
who don't have access to beefier machines to get WebKit built and even work
on patches. While this has not been very common, I think it's a commendable
goal to allow people without access to beefy computers to contribute to
WebKit :)

Best regards,
—Adrián



signature.asc
Description: PGP signature
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Suggesting to enable paint timing by default

[Keith Miller]

Results appear to be neutral on the page load time benchmark, so you should be 
good on that front. I don’t know who the best person to vet the maturity of the 
code is though, sorry.

Cheers,
Keith

> On Jul 13, 2020, at 11:38 AM, Noam Rosenthal  wrote:
> 
> 
> 
> On Mon, Jul 13, 2020 at 9:15 PM Keith Miller  > wrote:
> If you tell me how to enable paint timing by default, I can start an A/B task 
> for you. I’m probably not qualified to review it for code maturity though.
> Awesome, thanks!
> It's an experimental runtime flag calledPaintTimingEnabled
> I have a patch for enabling it by default here: 
> https://bugs.webkit.org/show_bug.cgi?id=211736 
> 
> We mainly need to test that measuring paint timing doesn't (badly) influence 
> loading performance.
> 
>   
> 
> Cheers,
> Keith
> 
>> On Jul 13, 2020, at 3:02 AM, Noam Rosenthal > > wrote:
>> 
>> 
>> 
>> On Wed, May 27, 2020 at 12:04 PM Yoav Weiss > > wrote:
>> +Ryosuke Niwa  +Alex Christensen 
>>  who were involved in the spec discussions.
>> 
>> On Wed, May 27, 2020 at 10:29 AM Noam Rosenthal > > wrote:
>> 
>> 
>> Following up on this.
>> FOn Tue, May 12, 2020 at 10:28 AM Maciej Stachowiak > > wrote:
>> 
>> 
>>> On May 11, 2020, at 9:53 PM, Noam Rosenthal >> > wrote:
>>> 
>>> 
>>> 
>>> On Tue, May 12, 2020 at 1:36 AM Maciej Stachowiak >> > wrote:
>>> 
>>> I noticed from comments in one of the Radars that the patch may result in 
>>> an additional “fake paint”, so it should probably be performance tested. 
>>> Have you done any testing? 
>>> I've tested it locally, I haven't noticed any significant side effect, 
>>> because in complex situations the fake paint only happens once per page and 
>>> bails early once contentfulness is detected. but I can run any additional 
>>> test needed.
>>>  
>>> We’ll likely want to A/B some of Apple’s page load speed benchmarks.
>>> A/B testing load speed sounds sensible. How do we go about doing that?
>> 
>> Unfortunately our page load speed benchmarks are not public because they 
>> incorporate captured page content, which we can’t freely redistribute.
>> 
>> So, can someone else from Apple review that the code is mature enough for 
>> this? Simon had reviewed the original patch. Maybe Zalan/Darin? 
>> 
>> A helpful person from Apple may be able to set up an A/B test for this patch.
>> What's required to ask for help from a helpful person at Apple? :) 
>> Hola
>> Pinging about this again :)
>> The code for paint timing API is sitting there in the repo, waiting either 
>> for internal Apple A/B tests, for an additional code maturity review, or for 
>> enabling it by default... I'm here if any changes in the code need to be 
>> made.
>> 
>> Trying to figure out how we can proceed with this... @Maciej Stachowiak 
>> ?
>> Cheers
>> ___
>> webkit-dev mailing list
>> webkit-dev@lists.webkit.org 
>> https://lists.webkit.org/mailman/listinfo/webkit-dev 
>> 

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] [webkit-changes] [264332] trunk/Source

[Darin Adler]

> On Jul 16, 2020, at 12:54 PM, Kirsling, Ross  wrote:
> 
> Non-unified build fixes are done to support *all* platforms, because sudden 
> unification shifts can happen anywhere.

I’m not sure that benefit is worth the additional code churn. I understand that 
it’s frustrating to run into a problem when unification shifts, say when you 
are adding a source file. I believe we have made changes to unification since 
the earliest version that reduce how often unification shifts.

— Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] [webkit-changes] [264332] trunk/Source

[Kirsling, Ross]

Non-unified build fixes are done to support *all* platforms, because sudden 
unification shifts can happen anywhere. We can't currently perform a full 
non-unified build on Mac since the CMake build only works up through JSC, so 
Sony and Igalia have been performing this maintenance by hand on Windows and 
Linux for years. It is for this reason that one is unlikely to encounter 
unification shift issues via EWS.

Tips for how to make better build fixes are helpful, but these build fixes 
can't go anywhere until we have a bot to identify missing includes and such as 
they arise.

Ross

From: Geoffrey Garen 
Sent: Thursday, July 16, 2020 8:54:00 AM
To: Yusuke Suzuki 
Cc: Fujii, Hironori (SIE) ; Darin Adler 
; WebKit Development Mailing List 
Subject: Re: [webkit-dev] [webkit-changes] [264332] trunk/Source

The original question was, as I understood it, was do we need to support 
non-unified builds as an essential requirement for a given port, and if so, why?

I’d like to finish answering that question, before we wonder off-topic and 
consider whether supporting non-unified builds as an optional way to improve 
our workflow is a good idea.

Thanks,
Geoff

On Jul 15, 2020, at 4:20 PM, Yusuke Suzuki 
mailto:ysuz...@apple.com>> wrote:

And I agree, keeping non-unified build sane is quite useful.
In addition to the benefit described by Alex, this also allows CMake to 
generate sane compile_commands.json, so that my completion in Neovim works 
better in cpp files,
and I think this compile_commands.json is also used in several clang-related 
toolings too.

I think we should have a bot maintaining it.

-Yusuke

On Jul 15, 2020, at 7:33 AM, Alex Christensen 
mailto:achristen...@apple.com>> wrote:

I think it is still quite useful to fix non-unified builds.  Otherwise adding a 
file usually involves many unrelated build fixes.

On Jul 14, 2020, at 5:25 PM, Fujii Hironori 
mailto:fujii.hiron...@gmail.com>> wrote:



On Wed, Jul 15, 2020 at 6:32 AM Sam Weinig 
mailto:wei...@apple.com>> wrote:
While I don’t want to take away from what Darin is saying here about correct 
usage of forward declaration and , I’d like to understand why we 
have two different compilation models supported in WebKit. Is there a reason 
both need to be supported? Can we remove one?


I also want to know that. Does anyone still need non-unified builds?

I introduced other unnecessary header inclusion, and Darin asked me to fix it.
https://bugs.webkit.org/show_bug.cgi?id=214204#c25
Reducing header inclusion can easily cause build breakages for non-unified 
builds.
So, I fixed non-unified build breakage in r264332 and r264333 as the 
preparation for that.
Non-unified builds was more broken than I expected. Does anyone still need it?
Should we maintain non-unified builds until C++20 modules will nullify unified 
builds?

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] [webkit-changes] [264332] trunk/Source

[Geoffrey Garen]

The original question was, as I understood it, was do we need to support 
non-unified builds as an essential requirement for a given port, and if so, why?

I’d like to finish answering that question, before we wonder off-topic and 
consider whether supporting non-unified builds as an optional way to improve 
our workflow is a good idea.

Thanks,
Geoff

> On Jul 15, 2020, at 4:20 PM, Yusuke Suzuki  wrote:
> 
> And I agree, keeping non-unified build sane is quite useful.
> In addition to the benefit described by Alex, this also allows CMake to 
> generate sane compile_commands.json, so that my completion in Neovim works 
> better in cpp files,
> and I think this compile_commands.json is also used in several clang-related 
> toolings too.
> 
> I think we should have a bot maintaining it.
> 
> -Yusuke
> 
>> On Jul 15, 2020, at 7:33 AM, Alex Christensen > > wrote:
>> 
>> I think it is still quite useful to fix non-unified builds.  Otherwise 
>> adding a file usually involves many unrelated build fixes.
>> 
>>> On Jul 14, 2020, at 5:25 PM, Fujii Hironori >> > wrote:
>>> 
>>> 
>>> 
>>> On Wed, Jul 15, 2020 at 6:32 AM Sam Weinig >> > wrote:
>>> While I don’t want to take away from what Darin is saying here about 
>>> correct usage of forward declaration and , I’d like to 
>>> understand why we have two different compilation models supported in 
>>> WebKit. Is there a reason both need to be supported? Can we remove one?
>>> 
>>> 
>>> I also want to know that. Does anyone still need non-unified builds?
>>> 
>>> I introduced other unnecessary header inclusion, and Darin asked me to fix 
>>> it.
>>> https://bugs.webkit.org/show_bug.cgi?id=214204#c25
>>>  Reducing header 
>>> inclusion can easily cause build breakages for non-unified builds.
>>> So, I fixed non-unified build breakage in r264332 and r264333 as the 
>>> preparation for that.
>>> Non-unified builds was more broken than I expected. Does anyone still need 
>>> it?
>>> Should we maintain non-unified builds until C++20 modules will nullify 
>>> unified builds?
>>>  
>>> ___
>>> webkit-dev mailing list
>>> webkit-dev@lists.webkit.org 
>>> https://lists.webkit.org/mailman/listinfo/webkit-dev 
>>> 
>> 
>> ___
>> webkit-dev mailing list
>> webkit-dev@lists.webkit.org 
>> https://lists.webkit.org/mailman/listinfo/webkit-dev
> 
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev