[Pharo 7.0-dev] Build #176: 20505-The-special-objects-array-needs-to-be-recreated-during-bootstrap

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[Pharo 7.0-dev] Build #176: 20505-The-special-objects-array-needs-to-be-recreated-during-bootstrap

ci-pharo-ci-jenkins2
There is a new Pharo build available!
       
The status of the build #176 was: SUCCESS.

The Pull Request #336 was integrated: "20505-The-special-objects-array-needs-to-be-recreated-during-bootstrap"
Pull request url: https://github.com/pharo-project/pharo/pull/336

Issue Url: https://pharo.fogbugz.com/f/cases/20505
Build Url: https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/development/176/
Reply | Threaded
Open this post in threaded view
|

Re: [Pharo 7.0-dev] Build #176: 20505-The-special-objects-array-needs-to-be-recreated-during-bootstrap

Guillermo Polito
Hi Pavel,

I don't like this change.

First, there is no comment nor explanation why that is needed. This change is obscure, if we forget in a couple of months why that line of code is there we may 
 - delete it unintentionally
 - or leave it forever and fear to clean it up because we do not understand it :(

Second, It looks that recreating the special objects array there, like that, means that it was not correctly created from the beginning. We should instead create it correctly from the beginning.

I think we should revert this change.

On Tue, Oct 10, 2017 at 12:24 PM, <[hidden email]> wrote:
There is a new Pharo build available!

The status of the build #176 was: SUCCESS.

The Pull Request #336 was integrated: "20505-The-special-objects-array-needs-to-be-recreated-during-bootstrap"
Pull request url: https://github.com/pharo-project/pharo/pull/336

Issue Url: https://pharo.fogbugz.com/f/cases/20505
Build Url: https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/development/176/



--

   

Guille Polito

Research Engineer

Centre de Recherche en Informatique, Signal et Automatique de Lille

CRIStAL - UMR 9189

French National Center for Scientific Research - http://www.cnrs.fr


Web: http://guillep.github.io

Phone: +33 06 52 70 66 13

Reply | Threaded
Open this post in threaded view
|

Re: [Pharo 7.0-dev] Build #176: 20505-The-special-objects-array-needs-to-be-recreated-during-bootstrap

Clément Béra
Hi Guille,

For "First":
I looked into this problem with Pavel this morning. The special object array being incorrectly created leads to problems when a large object (bigger that growthHeadroom) is created, as it does not go through the handleFailingBasicNew methods. Typically, Pavel could not load Seaside into the minimal image because of that. The problem lies with the initialization of error codes.

For "Second":
I don't mind what you do as long as it solves the problem of large allocations on minimal images. In any case the problems lies with the error code symbols, so that the code in basicNew: 
ec == #'insufficient object memory' ifTrue:
[^self handleFailingBasicNew: sizeRequested].
takes the correct branch. Before Pavel's change, the condition was false during bootstrap leading to large allocation crashing the VM, ec was equal to 9 instead of #'insufficient object memory', since the error codes in the special object array were incorrectly initialized somehow.

PS: Who is nobody ?

Cheers


On Tue, Oct 10, 2017 at 2:04 PM, Guillermo Polito <[hidden email]> wrote:
Hi Pavel,

I don't like this change.

First, there is no comment nor explanation why that is needed. This change is obscure, if we forget in a couple of months why that line of code is there we may 
 - delete it unintentionally
 - or leave it forever and fear to clean it up because we do not understand it :(

Second, It looks that recreating the special objects array there, like that, means that it was not correctly created from the beginning. We should instead create it correctly from the beginning.

I think we should revert this change.

On Tue, Oct 10, 2017 at 12:24 PM, <[hidden email]> wrote:
There is a new Pharo build available!

The status of the build #176 was: SUCCESS.

The Pull Request #336 was integrated: "20505-The-special-objects-array-needs-to-be-recreated-during-bootstrap"
Pull request url: https://github.com/pharo-project/pharo/pull/336

Issue Url: https://pharo.fogbugz.com/f/cases/20505
Build Url: https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/development/176/



--

   

Guille Polito

Research Engineer

Centre de Recherche en Informatique, Signal et Automatique de Lille

CRIStAL - UMR 9189

French National Center for Scientific Research - http://www.cnrs.fr


Web: http://guillep.github.io

Phone: <a href="tel:+33%206%2052%2070%2066%2013" value="+33652706613" target="_blank">+33 06 52 70 66 13




--
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq
Reply | Threaded
Open this post in threaded view
|

Re: [Pharo 7.0-dev] Build #176: 20505-The-special-objects-array-needs-to-be-recreated-during-bootstrap

Pavel Krivanek-3
In reply to this post by Guillermo Polito
I agree with you, it is too dirty solution. The next try:
Thank you for the pointers.

Cheers,
-- Pavel



2017-10-10 14:04 GMT+02:00 Guillermo Polito <[hidden email]>:
Hi Pavel,

I don't like this change.

First, there is no comment nor explanation why that is needed. This change is obscure, if we forget in a couple of months why that line of code is there we may 
 - delete it unintentionally
 - or leave it forever and fear to clean it up because we do not understand it :(

Second, It looks that recreating the special objects array there, like that, means that it was not correctly created from the beginning. We should instead create it correctly from the beginning.

I think we should revert this change.

On Tue, Oct 10, 2017 at 12:24 PM, <[hidden email]> wrote:
There is a new Pharo build available!

The status of the build #176 was: SUCCESS.

The Pull Request #336 was integrated: "20505-The-special-objects-array-needs-to-be-recreated-during-bootstrap"
Pull request url: https://github.com/pharo-project/pharo/pull/336

Issue Url: https://pharo.fogbugz.com/f/cases/20505
Build Url: https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/development/176/



--

   

Guille Polito

Research Engineer

Centre de Recherche en Informatique, Signal et Automatique de Lille

CRIStAL - UMR 9189

French National Center for Scientific Research - http://www.cnrs.fr


Web: http://guillep.github.io

Phone: <a href="tel:+33%206%2052%2070%2066%2013" value="+33652706613" target="_blank">+33 06 52 70 66 13


Reply | Threaded
Open this post in threaded view
|

Re: [Pharo 7.0-dev] Build #176: 20505-The-special-objects-array-needs-to-be-recreated-during-bootstrap

Guillermo Polito
In reply to this post by Clément Béra


On Tue, Oct 10, 2017 at 2:46 PM, Clément Bera <[hidden email]> wrote:
Hi Guille,

For "First":
I looked into this problem with Pavel this morning. The special object array being incorrectly created leads to problems when a large object (bigger that growthHeadroom) is created, as it does not go through the handleFailingBasicNew methods. Typically, Pavel could not load Seaside into the minimal image because of that. The problem lies with the initialization of error codes.

Yes we were discussing this with Pavel yesterday too. I pointed him to look for your blogpost on sunday when he would not load seaside :).

We I meant is that this should have been in a comment at least in the pull request. Otherwise your private discussion gets lost...
 

For "Second":
I don't mind what you do as long as it solves the problem of large allocations on minimal images. In any case the problems lies with the error code symbols, so that the code in basicNew: 
ec == #'insufficient object memory' ifTrue:
[^self handleFailingBasicNew: sizeRequested].
takes the correct branch. Before Pavel's change, the condition was false during bootstrap leading to large allocation crashing the VM, ec was equal to 9 instead of #'insufficient object memory', since the error codes in the special object array were incorrectly initialized somehow.

Yes, I understand the error.

Now, I was crunching it a bit yesterday, and this looks too error prone. This has a super hidden dependency with the special objects array.

I ask myself: do we want this to fail when the special object's array changes? What if somebody suddenly tries to "fix a typo" in the error message and forgots to update the special object's array?

Moreover, Why not doing

ec == ((Smalltalk specialObjectsArray at: 53) at: 9) ifTrue:
[^self handleFailingBasicNew: sizeRequested].

And why not adding also a guard in case ec == 9?

ec == 9 ifTrue:
[^self handleFailingBasicNew: sizeRequested].
ec == ((Smalltalk specialObjectsArray at: 53) at: 9) ifTrue:
[^self handleFailingBasicNew: sizeRequested].

Like that the fallback code could be a bit more robust.

What do you think?
 

PS: Who is nobody ?

Cheers


On Tue, Oct 10, 2017 at 2:04 PM, Guillermo Polito <[hidden email]> wrote:
Hi Pavel,

I don't like this change.

First, there is no comment nor explanation why that is needed. This change is obscure, if we forget in a couple of months why that line of code is there we may 
 - delete it unintentionally
 - or leave it forever and fear to clean it up because we do not understand it :(

Second, It looks that recreating the special objects array there, like that, means that it was not correctly created from the beginning. We should instead create it correctly from the beginning.

I think we should revert this change.

On Tue, Oct 10, 2017 at 12:24 PM, <[hidden email]> wrote:
There is a new Pharo build available!

The status of the build #176 was: SUCCESS.

The Pull Request #336 was integrated: "20505-The-special-objects-array-needs-to-be-recreated-during-bootstrap"
Pull request url: https://github.com/pharo-project/pharo/pull/336

Issue Url: https://pharo.fogbugz.com/f/cases/20505
Build Url: https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/development/176/



--

   

Guille Polito

Research Engineer

Centre de Recherche en Informatique, Signal et Automatique de Lille

CRIStAL - UMR 9189

French National Center for Scientific Research - http://www.cnrs.fr


Web: http://guillep.github.io

Phone: <a href="tel:+33%206%2052%2070%2066%2013" value="+33652706613" target="_blank">+33 06 52 70 66 13




--
Clément Béra
Pharo consortium engineer



--

   

Guille Polito

Research Engineer

Centre de Recherche en Informatique, Signal et Automatique de Lille

CRIStAL - UMR 9189

French National Center for Scientific Research - http://www.cnrs.fr


Web: http://guillep.github.io

Phone: +33 06 52 70 66 13