Smalltalk › Pharo › Pharo Smalltalk Developers

Strange Monticello bug: methods are not saved

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

28 messages Options

Nicolai Hess

Re: Strange Monticello bug: methods are not saved

2015-08-24 16:07 GMT+02:00 Thierry Goubier <[hidden email]>:

Hi Nicolai,

I think you're right. But why irregular errors?

Good question, important question, because we need to know what may have been wrongly imported.

can you check this:

MCMczReader>>loadDefinitions
    definitions := OrderedCollection new.
    (self zip memberNamed: 'snapshot.bin') ifNotNil:
        [:m | [^ definitions := (MCDataStream on: m contentStream) next definitions]
            on: Error do: [:fallThrough]].
    "otherwise"
    (self zip membersMatching: 'snapshot/*')
        do: [:m | self extractDefinitionsFrom: m].

and replace
on: Error do: [:fallThrough]].

with
on: Error do: [:fallThrough | self halt]].

some package can be loaded without error. But some throwing errors (for example: Error: Improper store into indexable object) and

the falltrough in the original code follow the

second "otherwise" path, and this is the reason why some package loadings modify the MCDefinition cache and some not.

Ok, for that particular method, with Zinc smalltalkhub repo open and Zn-Tests.231 selected (this forces a read of the mcz definitions in the image), what I have is:

ZnEntityTests>>#testUnspecifiedEncoding -> old version.
ok
MCMethodDefinition cachedDefinitions at: ZnEntityTests>>#testUnspecifiedEncoding -> new version
ok
ZnEntityTests>>#testUnspecifiedEncoding asRingDefinition -> old version
ok
ZnEntityTests>>#testUnspecifiedEncoding asRingDefinition asMCMethodDefinition -> new version
not ok.

Thierry

2015-08-24 15:37 GMT+02:00 Sven Van Caekenberghe <[hidden email]>:

> On 24 Aug 2015, at 15:31, Nicolai Hess <[hidden email]> wrote:
>
> Can someone look at
>
> RGMethodDefinition>>#asMCMethodDefinition
> I think this method uses one cache
> (MCMethodDefinition cachedDefinitions)
> even for both, methods from the MCPackage and methods from the image package,
> of course, they should not be equal. But they are index by the compiledMethod
> and this is always the compiled method of the existing method in the image (RGMethodDefinition>>compiledMethod)
>
> But if this is the cause of this bug, I would guess we see much more strange errors.....

Well, Stef reported a problem a couple of days ago, then Marcus another one, and now mine. All with versions, changes, code loading.

> 2015-08-24 15:06 GMT+02:00 Martin Dias <[hidden email]>:
>
> On Mon, Aug 24, 2015 at 3:03 PM, Thierry Goubier <[hidden email]> wrote:
>
>
> 2015-08-24 14:43 GMT+02:00 Sven Van Caekenberghe <[hidden email]>:
> I have to concur, something very strange is wrong.
>
> #50265
>
> Load Zinc-Tests-SvenVanCaekenberghe.231 from http://mc.stfx.eu/ZincHTTPComponents
>
> ZnEntityTests>>#testUnspecifiedEncoding should have today as latest version and it simply does not (the code is wrong too), I can't imagine how that is possible.
>
> And this is with loading code ... seems quite dangerous.
>
> Fun: copying Zinc-Tests-SvenVanCaekenberghe.231 to a filetree repo, then loading from the filetree is correct, at least for ZnEntityTests>>#testUnspecifiedEncoding
>
> #50265
>
> Strange. Once the MCDefinitions are loaded (repository type dependent, that), the code path is repository-independent, no?
>
> Strange. Does this happen for all changed methods, or only some of them?
>

Thierry Goubier

Re: Strange Monticello bug: methods are not saved

2015-08-24 16:53 GMT+02:00 Nicolai Hess <[hidden email]>:

2015-08-24 16:07 GMT+02:00 Thierry Goubier <[hidden email]>:
Hi Nicolai,

I think you're right. But why irregular errors?

Good question, important question, because we need to know what may have been wrongly imported.

can you check this:

MCMczReader>>loadDefinitions
    definitions := OrderedCollection new.
    (self zip memberNamed: 'snapshot.bin') ifNotNil:
        [:m | [^ definitions := (MCDataStream on: m contentStream) next definitions]
            on: Error do: [:fallThrough]].
    "otherwise"
    (self zip membersMatching: 'snapshot/*')
        do: [:m | self extractDefinitionsFrom: m].

and replace
on: Error do: [:fallThrough]].
with
on: Error do: [:fallThrough | self halt]].

some package can be loaded without error. But some throwing errors (for example: Error: Improper store into indexable object) and
the falltrough in the original code follow the
second "otherwise" path, and this is the reason why some package loadings modify the MCDefinition cache and some not.

I don't know enough of the adding path to be sure, but I wonder if the use of Ring as an intermediary to and from MCMethodDefinition isn't new, and exercising those paths. You may be pointing something else.

A short difference with Pharo4 shows that yes, Pharo5 goes through Ring and not Pharo4.

(see MCPackage>>#snapshot, changed in late june)

Thierry

Ok, for that particular method, with Zinc smalltalkhub repo open and Zn-Tests.231 selected (this forces a read of the mcz definitions in the image), what I have is:

ZnEntityTests>>#testUnspecifiedEncoding -> old version.
ok
MCMethodDefinition cachedDefinitions at: ZnEntityTests>>#testUnspecifiedEncoding -> new version
ok
ZnEntityTests>>#testUnspecifiedEncoding asRingDefinition -> old version
ok
ZnEntityTests>>#testUnspecifiedEncoding asRingDefinition asMCMethodDefinition -> new version
not ok.

Thierry

2015-08-24 15:37 GMT+02:00 Sven Van Caekenberghe <[hidden email]>:

> On 24 Aug 2015, at 15:31, Nicolai Hess <[hidden email]> wrote:
>
> Can someone look at
>
> RGMethodDefinition>>#asMCMethodDefinition
> I think this method uses one cache
> (MCMethodDefinition cachedDefinitions)
> even for both, methods from the MCPackage and methods from the image package,
> of course, they should not be equal. But they are index by the compiledMethod
> and this is always the compiled method of the existing method in the image (RGMethodDefinition>>compiledMethod)
>
> But if this is the cause of this bug, I would guess we see much more strange errors.....

Well, Stef reported a problem a couple of days ago, then Marcus another one, and now mine. All with versions, changes, code loading.

> 2015-08-24 15:06 GMT+02:00 Martin Dias <[hidden email]>:
>
> On Mon, Aug 24, 2015 at 3:03 PM, Thierry Goubier <[hidden email]> wrote:
>
>
> 2015-08-24 14:43 GMT+02:00 Sven Van Caekenberghe <[hidden email]>:
> I have to concur, something very strange is wrong.
>
> #50265
>
> Load Zinc-Tests-SvenVanCaekenberghe.231 from http://mc.stfx.eu/ZincHTTPComponents
>
> ZnEntityTests>>#testUnspecifiedEncoding should have today as latest version and it simply does not (the code is wrong too), I can't imagine how that is possible.
>
> And this is with loading code ... seems quite dangerous.
>
> Fun: copying Zinc-Tests-SvenVanCaekenberghe.231 to a filetree repo, then loading from the filetree is correct, at least for ZnEntityTests>>#testUnspecifiedEncoding
>
> #50265
>
> Strange. Once the MCDefinitions are loaded (repository type dependent, that), the code path is repository-independent, no?
>
> Strange. Does this happen for all changed methods, or only some of them?
>

stepharo

Re: Strange Monticello bug: methods are not saved

In reply to this post by Marcus Denker-4

I know that we got a similar behavior with franck. It looks like the
metadata of the package was confused.

Stef

Le 24/8/15 12:05, Marcus Denker a écrit :

> Hi,
>
> I have a *very* strange bug.
>
> I am trying to add change these three methods in class “TemporaryVariable”:
> (I copy the new version):
>
>
> ensureProperties
> method saveTemp: self.
> ^ Properties at: self ifAbsentPut: WeakKeyDictionary new
>
> removePropertiesIfEmpty
> ^ Properties at: self ifPresent: [ :dict |
> dict ifEmpty: [
> method removeSavedTemp: self.
> Properties removeKey: self ] ]
>
> hash
>
> ^ (name hash
> bitXor: method hash)
> bitXor: (startpc ifNil: 0)
>
>
> Now when I save the MC package, these changes are *not* saved, even though when I diff against the repository
> of Pharo5, they are shown as different.
>
> The only special things I did is to move TemporaryVariable to a new tag some updates ago. And I had extension
> methods in *slot where the same thing happened, but that was fixed by putting them into *Slot.
>
> This looks like some wired RPackage problem. But the RPackage seems to correctly know TemporaryVariable and these three selectors
> as local from the package Slot.
>
> I really have no idea what the problem could be.
>
> Marcus
>

stepharo

Re: Strange Monticello bug: methods are not saved

In reply to this post by Thierry Goubier

indeed we extracted a chunkImporter or something like that out of crappy code logic.

Le 24/8/15 17:08, Thierry Goubier a écrit :

2015-08-24 16:53 GMT+02:00 Nicolai Hess <[hidden email]>:

2015-08-24 16:07 GMT+02:00 Thierry Goubier <[hidden email]>:

Hi Nicolai,

I think you're right. But why irregular errors?

Good question, important question, because we need to know what may have been wrongly imported.

can you check this:

MCMczReader>>loadDefinitions
    definitions := OrderedCollection new.
    (self zip memberNamed: 'snapshot.bin') ifNotNil:
        [:m | [^ definitions := (MCDataStream on: m contentStream) next definitions]
            on: Error do: [:fallThrough]].
    "otherwise"
    (self zip membersMatching: 'snapshot/*')
        do: [:m | self extractDefinitionsFrom: m].

and replace
on: Error do: [:fallThrough]].

with
on: Error do: [:fallThrough | self halt]].

some package can be loaded without error. But some throwing errors (for example: Error: Improper store into indexable object) and

the falltrough in the original code follow the

second "otherwise" path, and this is the reason why some package loadings modify the MCDefinition cache and some not.

I don't know enough of the adding path to be sure, but I wonder if the use of Ring as an intermediary to and from MCMethodDefinition isn't new, and exercising those paths. You may be pointing something else.

A short difference with Pharo4 shows that yes, Pharo5 goes through Ring and not Pharo4.

(see MCPackage>>#snapshot, changed in late june)

Thierry

Ok, for that particular method, with Zinc smalltalkhub repo open and Zn-Tests.231 selected (this forces a read of the mcz definitions in the image), what I have is:

ZnEntityTests>>#testUnspecifiedEncoding -> old version.

ok

MCMethodDefinition cachedDefinitions at: ZnEntityTests>>#testUnspecifiedEncoding -> new version

ok

ZnEntityTests>>#testUnspecifiedEncoding asRingDefinition -> old version

ok

ZnEntityTests>>#testUnspecifiedEncoding asRingDefinition asMCMethodDefinition -> new version

not ok.

Thierry

2015-08-24 15:37 GMT+02:00 Sven Van Caekenberghe <[hidden email]>:

> On 24 Aug 2015, at 15:31, Nicolai Hess <[hidden email]> wrote:
>
> Can someone look at
>
> RGMethodDefinition>>#asMCMethodDefinition
> I think this method uses one cache
> (MCMethodDefinition cachedDefinitions)
> even for both, methods from the MCPackage and methods from the image package,
> of course, they should not be equal. But they are index by the compiledMethod
> and this is always the compiled method of the existing method in the image (RGMethodDefinition>>compiledMethod)
>
> But if this is the cause of this bug, I would guess we see much more strange errors.....

Well, Stef reported a problem a couple of days ago, then Marcus another one, and now mine. All with versions, changes, code loading.

> 2015-08-24 15:06 GMT+02:00 Martin Dias <[hidden email]>:
>
> On Mon, Aug 24, 2015 at 3:03 PM, Thierry Goubier <[hidden email]> wrote:
>
>
> 2015-08-24 14:43 GMT+02:00 Sven Van Caekenberghe <[hidden email]>:
> I have to concur, something very strange is wrong.
>
> #50265
>
> Load Zinc-Tests-SvenVanCaekenberghe.231 from http://mc.stfx.eu/ZincHTTPComponents
>
> ZnEntityTests>>#testUnspecifiedEncoding should have today as latest version and it simply does not (the code is wrong too), I can't imagine how that is possible.
>
> And this is with loading code ... seems quite dangerous.
>
> Fun: copying Zinc-Tests-SvenVanCaekenberghe.231 to a filetree repo, then loading from the filetree is correct, at least for ZnEntityTests>>#testUnspecifiedEncoding
>
> #50265
>
> Strange. Once the MCDefinitions are loaded (repository type dependent, that), the code path is repository-independent, no?
>
> Strange. Does this happen for all changed methods, or only some of them?
>

stepharo

Re: Strange Monticello bug: methods are not saved

In reply to this post by Thierry Goubier

This is really nice to have people like you guys around.
This is a really nice example where the sum is more than its elements

Stef

Le 24/8/15 16:44, Thierry Goubier a écrit :

2015-08-24 16:32 GMT+02:00 Marcus Denker <[hidden email]>:

>
> solves loading Sven test packages. Marcus, can you try on yours?
>

Yes! Very good!

Slice pushed to 16358. We'll see what the tests say.

Why has it started to fail? All the involved code is old.

Thierry

Marcus

Nicolai Hess

Re: Strange Monticello bug: methods are not saved

2015-08-24 18:21 GMT+02:00 stepharo <[hidden email]>:

This is really nice to have people like you guys around.
This is a really nice example where the sum is more than its elements

Stef

Le 24/8/15 16:44, Thierry Goubier a écrit :

2015-08-24 16:32 GMT+02:00 Marcus Denker <[hidden email]>:

>
> solves loading Sven test packages. Marcus, can you try on yours?
>

Yes! Very good!

Slice pushed to 16358. We'll see what the tests say.

Why has it started to fail? All the involved code is old.

Thierry

Marcus

Great, Thierrys solution works.

But I think there is still a problem with RGMethodDefinition. We call

self compiledMethod

on a RGMethodDefintion, the implementation *always* returns the compiled method from the system, if

it exists (regardless whether this ring method is active or not).

I think we should either

- don't call #compiledMethod on ring definitions

- always return nil for not active definitions

- for non active definitions: create a compiled method on demand

what do you think?

Marcus Denker-4

Re: Strange Monticello bug: methods are not saved

Great, Thierrys solution works.

But I think there is still a problem with RGMethodDefinition. We call
self compiledMethod
on a RGMethodDefintion, the implementation *always* returns the compiled method from the system, if
it exists (regardless whether this ring method is active or not).

I think we should either
- don't call #compiledMethod on ring definitions
- always return nil for not active definitions
- for non active definitions: create a compiled method on demand

what do you think?

Returning nil might be good… because non-active ones model methods that are not installed

and have no compiled method.

But then, the problem is that we use non active Ring definitions as a replacement of what was

MethodReference… so we need to check if that breaks anything.

Another thing we need to check is all uses of Ring: e.g. SystemNavigation. Do we need

Ring there?

In general, we need to do another pass over Ring in the context of the remote

development project.

Marcus

Thierry Goubier

Re: Strange Monticello bug: methods are not saved

In reply to this post by Nicolai Hess

2015-08-25 8:19 GMT+02:00 Nicolai Hess <[hidden email]>:

2015-08-24 18:21 GMT+02:00 stepharo <[hidden email]>:

This is really nice to have people like you guys around.
This is a really nice example where the sum is more than its elements

Stef

Le 24/8/15 16:44, Thierry Goubier a écrit :

2015-08-24 16:32 GMT+02:00 Marcus Denker <[hidden email]>:

>
> solves loading Sven test packages. Marcus, can you try on yours?
>

Yes! Very good!

Slice pushed to 16358. We'll see what the tests say.

Why has it started to fail? All the involved code is old.

Thierry

Marcus

Great, Thierrys solution works.

But I think there is still a problem with RGMethodDefinition. We call
self compiledMethod
on a RGMethodDefintion, the implementation *always* returns the compiled method from the system, if
it exists (regardless whether this ring method is active or not).

I think we should either
- don't call #compiledMethod on ring definitions
- always return nil for not active definitions
- for non active definitions: create a compiled method on demand

what do you think?

It depends what we want Ring to be. If it is a replacement to MCDefinition and all, then it must handle non-active methods. If it models the live system as well, then it must answers compiledMethod.

I would choose return nil or return an exception.

Third one could be interesting in some contexts... running packages without loading them. But would be hard to do correctly.

Thierry