| Message ID | 20230217095855.1932015-1-jose.quaresma@foundries.io |
|---|---|
| State | Accepted, archived |
| Commit | 0fd3a9c13a30a67ccef6619627efd9613755a0c3 |
| Headers | show |
| Series | icecc.bbclass: enable the network only when ICECC_DISABLED is not set | expand |
On Fri, 2023-02-17 at 09:58 +0000, Jose Quaresma wrote: > Enabling the network uncondictional is not need for some use cases. > > Such use case is usefull to reuse the sstate-cache of the build > and it requires the icecc inherit in all of the builds. > The real control control of the icecc is in the variable ICECC_DISABLED > so this patch change the logic to enable the network when the icecc is in use. > > Signed-off-by: Jose Quaresma <jose.quaresma@foundries.io> > --- > meta/classes/icecc.bbclass | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/meta/classes/icecc.bbclass b/meta/classes/icecc.bbclass > index 312e0f17b5..159cae20f8 100644 > --- a/meta/classes/icecc.bbclass > +++ b/meta/classes/icecc.bbclass > @@ -428,22 +428,18 @@ set_icecc_env() { > bbnote "Using icecc tarball: $ICECC_VERSION" > } > > -do_configure[network] = "1" > do_configure:prepend() { > set_icecc_env > } > > -do_compile[network] = "1" > do_compile:prepend() { > set_icecc_env > } > > -do_compile_kernelmodules[network] = "1" > do_compile_kernelmodules:prepend() { > set_icecc_env > } > > -do_install[network] = "1" > do_install:prepend() { > set_icecc_env > } > @@ -457,3 +453,9 @@ ICECC_SDK_HOST_TASK:pn-uninative-tarball = "" > > # Add the toolchain scripts to the SDK > TOOLCHAIN_HOST_TASK:append = " ${ICECC_SDK_HOST_TASK}" > + > +python () { > + if d.getVar('ICECC_DISABLED') != "1": > + for task in ['do_configure', 'do_compile', 'do_compile_kernelmodules', 'do_install']: > + d.setVarFlag(task, 'network', '1') > +} I did want to talk a little about the overhead this has. Basically you've turned a simple flag setting into some anonymous python so the parsing overhead increases with it since there is now code parsing and execution to add in. For a single recipe this isn't so bad but when it is being added to every recipe it adds up. I'm not against the change as such, I just see a continual flood of patches adding conditions everywhere for various reasons and each one degrades parsing speed a little bit more (as well as complicates the test matrix). I personally don't use this class so I don't have a stake in this one but I thought it worth highlighting since I think the overhead of this was mentioned elsewhere and I think the overhead isn't widely understood. Cheers, Richard
On Fri, Feb 24, 2023 at 5:27 PM Richard Purdie < richard.purdie@linuxfoundation.org> wrote: > On Fri, 2023-02-17 at 09:58 +0000, Jose Quaresma wrote: > > Enabling the network uncondictional is not need for some use cases. > > > > Such use case is usefull to reuse the sstate-cache of the build > > and it requires the icecc inherit in all of the builds. > > The real control control of the icecc is in the variable ICECC_DISABLED > > so this patch change the logic to enable the network when the icecc is > in use. > > > > Signed-off-by: Jose Quaresma <jose.quaresma@foundries.io> > > --- > > meta/classes/icecc.bbclass | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/meta/classes/icecc.bbclass b/meta/classes/icecc.bbclass > > index 312e0f17b5..159cae20f8 100644 > > --- a/meta/classes/icecc.bbclass > > +++ b/meta/classes/icecc.bbclass > > @@ -428,22 +428,18 @@ set_icecc_env() { > > bbnote "Using icecc tarball: $ICECC_VERSION" > > } > > > > -do_configure[network] = "1" > > do_configure:prepend() { > > set_icecc_env > > } > > > > -do_compile[network] = "1" > > do_compile:prepend() { > > set_icecc_env > > } > > > > -do_compile_kernelmodules[network] = "1" > > do_compile_kernelmodules:prepend() { > > set_icecc_env > > } > > > > -do_install[network] = "1" > > do_install:prepend() { > > set_icecc_env > > } > > @@ -457,3 +453,9 @@ ICECC_SDK_HOST_TASK:pn-uninative-tarball = "" > > > > # Add the toolchain scripts to the SDK > > TOOLCHAIN_HOST_TASK:append = " ${ICECC_SDK_HOST_TASK}" > > + > > +python () { > > + if d.getVar('ICECC_DISABLED') != "1": > > + for task in ['do_configure', 'do_compile', > 'do_compile_kernelmodules', 'do_install']: > > + d.setVarFlag(task, 'network', '1') > > +} > > I did want to talk a little about the overhead this has. Basically > you've turned a simple flag setting into some anonymous python so the > parsing overhead increases with it since there is now code parsing and > execution to add in. > > For a single recipe this isn't so bad but when it is being added to > every recipe it adds up. > > I'm not against the change as such, I just see a continual flood of > patches adding conditions everywhere for various reasons and each one > degrades parsing speed a little bit more (as well as complicates the > test matrix). > > I personally don't use this class so I don't have a stake in this one > but I thought it worth highlighting since I think the overhead of this > was mentioned elsewhere and I think the overhead isn't widely > understood. > As the network evaluation patch was merged in bitbake: https://git.openembedded.org/bitbake/commit/?id=3d96c07f9fd4ba1a84826811d14bb4e98ad58846 I can send my inline version: https://git.openembedded.org/openembedded-core-contrib/commit/?h=jansa/master&id=1b143f065ab5408f5543951a8c73fa01ce3ee0c1 This works for master, for langdale and kirkstone we would need to backport this bitbake commit as well as "utils: Allow to_boolean to support int values", I have all 3 in my branches for kirkstone and langdale and can send them as soon as there is agreement to do that (e.g. top 5 changes from https://git.openembedded.org/bitbake-contrib/log/?h=jansa/2.0) Regards,
On Fri, 2023-02-24 at 17:38 +0100, Martin Jansa wrote: > On Fri, Feb 24, 2023 at 5:27 PM Richard Purdie > <richard.purdie@linuxfoundation.org> wrote: > > On Fri, 2023-02-17 at 09:58 +0000, Jose Quaresma wrote: > > > Enabling the network uncondictional is not need for some use > > > cases. > > > > > > Such use case is usefull to reuse the sstate-cache of the build > > > and it requires the icecc inherit in all of the builds. > > > The real control control of the icecc is in the variable > > > ICECC_DISABLED > > > so this patch change the logic to enable the network when the > > > icecc is in use. > > > > > > Signed-off-by: Jose Quaresma <jose.quaresma@foundries.io> > > > --- > > > meta/classes/icecc.bbclass | 10 ++++++---- > > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > > > diff --git a/meta/classes/icecc.bbclass > > > b/meta/classes/icecc.bbclass > > > index 312e0f17b5..159cae20f8 100644 > > > --- a/meta/classes/icecc.bbclass > > > +++ b/meta/classes/icecc.bbclass > > > @@ -428,22 +428,18 @@ set_icecc_env() { > > > bbnote "Using icecc tarball: $ICECC_VERSION" > > > } > > > > > > -do_configure[network] = "1" > > > do_configure:prepend() { > > > set_icecc_env > > > } > > > > > > -do_compile[network] = "1" > > > do_compile:prepend() { > > > set_icecc_env > > > } > > > > > > -do_compile_kernelmodules[network] = "1" > > > do_compile_kernelmodules:prepend() { > > > set_icecc_env > > > } > > > > > > -do_install[network] = "1" > > > do_install:prepend() { > > > set_icecc_env > > > } > > > @@ -457,3 +453,9 @@ ICECC_SDK_HOST_TASK:pn-uninative-tarball = "" > > > > > > # Add the toolchain scripts to the SDK > > > TOOLCHAIN_HOST_TASK:append = " ${ICECC_SDK_HOST_TASK}" > > > + > > > +python () { > > > + if d.getVar('ICECC_DISABLED') != "1": > > > + for task in ['do_configure', 'do_compile', > > > 'do_compile_kernelmodules', 'do_install']: > > > + d.setVarFlag(task, 'network', '1') > > > +} > > > > I did want to talk a little about the overhead this has. Basically > > you've turned a simple flag setting into some anonymous python so > > the > > parsing overhead increases with it since there is now code parsing > > and > > execution to add in. > > > > For a single recipe this isn't so bad but when it is being added to > > every recipe it adds up. > > > > I'm not against the change as such, I just see a continual flood of > > patches adding conditions everywhere for various reasons and each > > one > > degrades parsing speed a little bit more (as well as complicates > > the > > test matrix). > > > > I personally don't use this class so I don't have a stake in this > > one > > but I thought it worth highlighting since I think the overhead of > > this > > was mentioned elsewhere and I think the overhead isn't widely > > understood. > > > > > As the network evaluation patch was merged in bitbake: > https://git.openembedded.org/bitbake/commit/?id=3d96c07f9fd4ba1a84826811d14bb4e98ad58846 > > I can send my inline version: > https://git.openembedded.org/openembedded-core-contrib/commit/?h=jansa/master&id=1b143f065ab5408f5543951a8c73fa01ce3ee0c1 > > This works for master, for langdale and kirkstone we would need to > backport this bitbake commit as well as "utils: Allow to_boolean to > support int values", I have all 3 in my branches for kirkstone and > langdale and can send them as soon as there is agreement to do that > (e.g. top 5 changes from > https://git.openembedded.org/bitbake-contrib/log/?h=jansa/2.0) Just to explain things for the purposes of the archives, that inline patch, whilst quite neat looking, will perform worse than the anonymous python. The reason is relatively simple. The anonymous python will run once, the inline will cause 4 different python fragments to run at best when the variable is expanded. Bitbake will have to expand it to compute the recipes basehashes. In reality, the expansion cache is often invalidated by datastore write operations so it can be expanded multiple times. I'm also reluctant to backport a performance regression and change in behaviour back to stable branches so Jose's patch may be the better option. Cheers, Richard
diff --git a/meta/classes/icecc.bbclass b/meta/classes/icecc.bbclass index 312e0f17b5..159cae20f8 100644 --- a/meta/classes/icecc.bbclass +++ b/meta/classes/icecc.bbclass @@ -428,22 +428,18 @@ set_icecc_env() { bbnote "Using icecc tarball: $ICECC_VERSION" } -do_configure[network] = "1" do_configure:prepend() { set_icecc_env } -do_compile[network] = "1" do_compile:prepend() { set_icecc_env } -do_compile_kernelmodules[network] = "1" do_compile_kernelmodules:prepend() { set_icecc_env } -do_install[network] = "1" do_install:prepend() { set_icecc_env } @@ -457,3 +453,9 @@ ICECC_SDK_HOST_TASK:pn-uninative-tarball = "" # Add the toolchain scripts to the SDK TOOLCHAIN_HOST_TASK:append = " ${ICECC_SDK_HOST_TASK}" + +python () { + if d.getVar('ICECC_DISABLED') != "1": + for task in ['do_configure', 'do_compile', 'do_compile_kernelmodules', 'do_install']: + d.setVarFlag(task, 'network', '1') +}
Enabling the network uncondictional is not need for some use cases. Such use case is usefull to reuse the sstate-cache of the build and it requires the icecc inherit in all of the builds. The real control control of the icecc is in the variable ICECC_DISABLED so this patch change the logic to enable the network when the icecc is in use. Signed-off-by: Jose Quaresma <jose.quaresma@foundries.io> --- meta/classes/icecc.bbclass | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)