Re: [squeak-dev] Re: [Vm-dev] Bug in writing compressed stream when saving an mcz (was: New Cog VMs available...)

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

Re: [squeak-dev] Re: [Vm-dev] Bug in writing compressed stream when saving an mcz (was: New Cog VMs available...)

Eliot Miranda-2
 



On Fri, Jul 18, 2014 at 12:38 PM, Eliot Miranda <[hidden email]> wrote:
Stupid me.  It's got to be the DeflateStream, and its subclasses ZipWriteStream, GZipWriteStream and ZLibWriteStream.  So my change screws all uses of these in plugins.  So what to do about fixing this?  I would like to have a go at fixing the plugins.  But cheaper might be to fix WriteStream with some property hack, storing the initialPosition somewhere else (a class side weak dictionary of stream -> initialPosition ?? (yuck)).


On Fri, Jul 18, 2014 at 12:31 PM, Eliot Miranda <[hidden email]> wrote:



On Thu, Jul 17, 2014 at 4:02 PM, David T. Lewis <[hidden email]> wrote:
On Fri, Jul 18, 2014 at 12:24:29AM +0200, Nicolas Cellier wrote:
> 2014-07-13 16:22 GMT+02:00 David T. Lewis <[hidden email]>:
>
> > On Sun, Jul 13, 2014 at 09:55:41AM -0400, David T. Lewis wrote:
> > > On Sun, Jul 06, 2014 at 12:47:25PM -0400, David T. Lewis wrote:
> > > > On Sun, Jul 06, 2014 at 12:23:11PM -0400, David T. Lewis wrote:
> > > > > On Sun, Jul 06, 2014 at 10:34:15AM +0200, Nicolas Cellier wrote:
> > > > > > 2014-07-04 0:47 GMT+02:00 Nicolas Cellier <
> > > > > > [hidden email]>:
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > 2014-07-04 0:09 GMT+02:00 Eliot Miranda <[hidden email]
> > >:
> > > > > > >
> > > > > > >
> > > > > > >>
> > > > > > >>
> > > > > > >> On Thu, Jul 3, 2014 at 12:33 PM, Nicolas Cellier <
> > > > > > >> [hidden email]> wrote:
> > > > > > >>
> > > > > > >>> The bug is repeatable, i simply have to execute this snippet
> > with my
> > > > > > >>> test file:
> > > > > > >>>
> > > > > > >>> (FileStream fileNamed: 'snapshot.bin') binary
> > contentsOfEntireFile
> > > > > > >>> asString zipped.
> > > > > > >>>
> > > > > > >>> The file is too big for an attachment here - 7.3 Mbytes - or
> > 1.7 Mbytes
> > > > > > >>> gzipped by external tool.
> > > > > > >>> If someone can suggest an upload site, or want it by mail,
> > just ask.
> > > > > > >>>
> > > > > > >>
> > > > > > >> You're welcome to put it on ftp.mirandanamda.org, cogftpuser,
> > pw cogging
> > > > > > >> with 0's & 1's.
> > > > > > >>
> > > > > > >>
> > > > > > > done, pollution of your site engaged...
> > > > > > > Thanks Eliot!
> > > > > > >
> > > > > > >
> > > > > > >>
> > > > > > I inquired a bit more about this bug.
> > > > > > An important clue is that it does not happen in Pharo3.0!
> > > > > >
> > > > > > But Pharo3.0 did not fundamentally change compression
> > > > > > (except some FileSystem related changes, separation of CRC stuff
> > in another
> > > > > > package, some other cosmetic changes like replacing some self
> > assert: ...
> > > > > > by [...] assert and a potential bug in
> > > > > > DeflateStream>>nextPutAll:startingAt: introduced by CamilleTeruel).
> > > > > > Squeak behavior is presumably not due to a Compression change.
> > > > > >
> > > > > > Neither is it a VM problem, the bugs still shows up when running
> > the Squeak
> > > > > > image with Pharo VM.
> > > > > >
> > > > >
> > > > > Yes, it is definite a problem in the Squeak trunk image.
> > > > >
> > > > >
> > > > > > So the difference lies somewhere else: in our beloved Stream.
> > > > > > I remembered a recent change of Eliot related to handling of
> > Stream created
> > > > > > with on:from:to: (Collections-eem.567).
> > > > > > Reverting those changes just make the snippet pass!
> > > > > >
> > > > > > Ah ah! At the time, i found the change of Eliot quite minimal and
> > nice.
> > > > > > But I wish I had time to analyze this small innocent change deeper.
> > > > > > Indeed, I remembered I previously broke my teeth on this one 2
> > years or so
> > > > > > ago, and preferred to adopt another strategy: avoid using
> > on:from:to: and
> > > > > > ReadWriteStream as much as possible.
> > > > > > Why? because when analyzing the usage of inst.var. in Stream
> > hierarchy, it
> > > > > > gave me headaches, some subclass are just ignoring superclass
> > variables, or
> > > > > > worse are bending the semantics of superclass variables.
> > > > > > I came to the conclusion that I could not decently master a change
> > of
> > > > > > on:from:to:
> > > > >
> > > > > I can not confirm this. I have an image in which I can reliably
> > reproduce the
> > > > > failure in writing the MCZ to a file. I tried reverting the methods
> > that were
> > > > > introduced in Collections-eem.567 and I am still getting the same
> > failure.
> > > > >
> > > > > Dave
> > > >
> > > > Oops, I spoke too soon. Indeed the problem appears as of
> > Collections-eem.567,
> > > > and does not appear in Collections-nice.566.
> > > >
> > > > So I am confirming Nicolas' observations ... now to find the bug :)
> > > >
> > > > Dave
> > > >
> > >
> > > This seems to be a rather tricky bug, and I don't think any of us knows
> > the
> > > cause right now. In case anyone else wants to have a look at it, here is
> > a
> > > recipe for reproducing the bug in a clean 4.5 image:
> > >
> > > - Start with a clean Squeak 4.5 image in and empty working directory.
> > >       ftp://ftp.squeak.org/4.5/Squeak4.5-13680.zip
> > >
> > > - Open the image, do not update it.
> > >
> > > - Open an MC browser and add the http://source.squeak.org/trunk
> > repository for the Help-Squeak-Project package and Collections package.
> > >
> > > - Load Help-Squeak-Project-kfr.18 from trunk.
> > >
> > > - Load Collections-eem.567 from trunk.
> > >
> > > - Open a browser on class SqueakToolsDebuggerHelp, and delete the class
> > side method #showDebuggerMenuForm.
> > >
> > > - Remove the http://source.squeak.org/trunk repository form the
> > Help-Squeak-Project package in the MC browser.
> > >
> > > - Highlight your package-cache repository and try to save
> > Help-Squeak-project to the package-cache repository.
> > >
> > > - Enter author initials 'dtl'.
> > >
> > > - For the package comment, enter 'Remove unreferenced method'.
> > >
> > > - Package save will fail part way through saving the MCZ.
> > >
> > > - To reproduce the failure in this image, do the following in a
> > workspace:
> > >
> > >       mcv := MCVersion allInstances detect: [:e | e name = 'a
> > MCVersion(Help-Squeak-Project-dtl.19)'].
> > >       [f := FileStream fileNamed: 'junk.mcz'.
> > >       mcv fileOutOn: f]
> > >               ensure: [f close].
> > >
> >
> > In addition to the above recipe, here are some other observations:
> >
> > - Reverting from Collections-eem.567 to Collections-nice.566 makes the
> >   problem go away, but ONLY if the DeflatePlugin is active.
> >
> > - The failure appears while executing the fallback code for
> > #primitiveDeflateBlock
> >   in ZipWriteStream>>deflateBlock:chainLength:goodMatch:
> >
> > - If this primitive is deactivate by commenting out the
> >   <primitive: 'primitiveDeflateBlock' module: 'ZipPlugin'> in
> >   ZipWriteStream>>deflateBlock:chainLength:goodMatch: then the problem will
> >   occur regardless of which version of Collections is loaded.
> >
> > - When Collections-eem.567 is loaded, the primitive will fail on the 22nd
> >   call to #deflateBlock:chainLength:goodMatch: and a debugger appears
> > during
> >   execution of the fallback code.
> >
> > - When Collections-nice.566 is loaded, the primitive does not fail on the
> > 22nd
> >   call to #deflateBlock:chainLength:goodMatch:, the fallback code is never
> > run,
> >   and no error appears.
> >
> > - When Collections-nice.566 is loaded and the primitive is disabled to
> > force
> >   use of the fallback code, the error appears on on the 22nd call to
> >   #deflateBlock:chainLength:goodMatch: in exactlly the same place as when
> >   Collections-eem.567 is loaded.
> >
> > - If I inspect the ZipWriteStream at the failure point (when the debugger
> > pops
> >   up after the problem), I see no obvious difference in the state of the
> > stream
> >   between the Collections-eem.567 case versus the Collections-nice.566 with
> >   primitive disabled case.
> >
> > Dave
> >
> >
> Very good mining work!
> When I hear  Stream, I think fluid...
> But these snags make it so viscous, it's like our Stream is going to freeze
> soon.
> Or is it more than frozen? The vein you're after sounds as hard as diamond,
> we gonna need a sharp peak pickaxe.
>

I keep hoping that your stream fixes will somehow make this problem go away, but
maybe we are not so lucky.

Whenever I get more time to play with this, I think I will try to catch it with
gdb in the primitive. There is something strange happening that seems to first
be detectable in the primitive failure, but only if Eliot's image fix is present.

This kind of problem is a nice way to spend the morning with a good cup of coffee,
as some people do with the NY Times crossword puzzle ;)

   http://en.wikipedia.org/wiki/The_New_York_Times_crossword_puzzle

Another "morning coffee" project is to figure out which version of the LargeIntegersPlugin
should be used, which of course was the original topic that started this thread.
 
So given that the change adds an instance variable to WriteStream could that affect some plugin that somehow accesses a stream?  What plugin could that be?  I don't see any obvious inst vars in the DeflatePlugin or InflatePlugin.  To remind ourselves, the three changes are

add inst var to WriteStream:

PositionableStream subclass: #WriteStream
instanceVariableNames: 'writeLimit initialPositionOrNil'
classVariableNames: ''
poolDictionaries: ''
category: 'Collections-Streams'

use it in two places:
contents
"Answer with a copy of my collection from the start to the current position."

readLimit := readLimit max: position.
^collection copyFrom: (initialPositionOrNil ifNil: [1]) to: position

on: aCollection from: firstIndex to: lastIndex

| len |
collection := aCollection.
readLimit := 
writeLimit := lastIndex > (len := collection size)
ifTrue: [len]
ifFalse: [lastIndex].
position := firstIndex <= 1
ifTrue: [0]
ifFalse: [firstIndex - 1].
initialPositionOrNil := position + 1

What I've just found is that if I revert all three changes to WriteStream the bug goes away, but if I only revert the two method changes the bug remains.  So I think the problem is merely the adding of the inst var.  This could break some plugin which was expecting inst vars in subclasses of WriteStream to be at particular offsets determined by WriteStream instSize = 4 now being 5.  The question is which plugin(s)?

Doh!
 
DeflatePlugin>>loadDeflateStreamFrom: rcvr
| oop |
<inline: false>
((interpreterProxy isPointers: rcvr)
and: [(interpreterProxy slotSizeOf: rcvr) >= 15]) ifFalse:
[^false].
oop := interpreterProxy fetchPointer: 0 ofObject: rcvr.
(interpreterProxy isBytes: oop) ifFalse:
[^false].
zipCollection := interpreterProxy firstIndexableField: oop.
zipCollectionSize := interpreterProxy byteSizeOf: oop.

zipPosition := interpreterProxy fetchInteger: 1 ofObject: rcvr.
zipReadLimit := interpreterProxy fetchInteger: 2 ofObject: rcvr.
"zipWriteLimit := interpreterProxy fetchInteger: 3 ofObject: rcvr."

oop := interpreterProxy fetchPointer: 4 ofObject: rcvr.
((interpreterProxy isWords: oop)
and: [(interpreterProxy slotSizeOf: oop) = DeflateHashTableSize]) ifFalse:
[^false].
zipHashHead := interpreterProxy firstIndexableField: oop.
oop := interpreterProxy fetchPointer: 5 ofObject: rcvr.
((interpreterProxy isWords: oop)
and: [(interpreterProxy slotSizeOf: oop) = DeflateWindowSize]) ifFalse:
[^false].
zipHashTail := interpreterProxy firstIndexableField: oop.
zipHashValue := interpreterProxy fetchInteger: 6 ofObject: rcvr.
zipBlockPos := interpreterProxy fetchInteger: 7 ofObject: rcvr.
"zipBlockStart := interpreterProxy fetchInteger: 8 ofObject: rcvr."
oop := interpreterProxy fetchPointer: 9 ofObject: rcvr.
(interpreterProxy isBytes: oop) ifFalse:
[^false].
zipLiteralSize := interpreterProxy slotSizeOf: oop.
zipLiterals := interpreterProxy firstIndexableField: oop.

oop := interpreterProxy fetchPointer: 10 ofObject: rcvr.
((interpreterProxy isWords: oop)
and: [(interpreterProxy slotSizeOf: oop) >= zipLiteralSize]) ifFalse:
[^false].
zipDistances := interpreterProxy firstIndexableField: oop.

oop := interpreterProxy fetchPointer: 11 ofObject: rcvr.
((interpreterProxy isWords: oop)
and: [(interpreterProxy slotSizeOf: oop) = DeflateMaxLiteralCodes]) ifFalse:
[^false].
zipLiteralFreq := interpreterProxy firstIndexableField: oop.

oop := interpreterProxy fetchPointer: 12 ofObject: rcvr.
((interpreterProxy isWords: oop)
and: [(interpreterProxy slotSizeOf: oop) = DeflateMaxDistanceCodes]) ifFalse:
[^false].
zipDistanceFreq := interpreterProxy firstIndexableField: oop.

zipLiteralCount := interpreterProxy fetchInteger: 13 ofObject: rcvr.
zipMatchCount := interpreterProxy fetchInteger: 14 ofObject: rcvr.

^interpreterProxy failed not

I propose to add a variable that holds the inst size of the superclass of InflateStream, and to use this to correct the offsets.  Objections?
--
best,
Eliot
Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] Re: [Vm-dev] Bug in writing compressed stream when saving an mcz (was: New Cog VMs available...)

David T. Lewis
 

>  On Fri, Jul 18, 2014 at 12:38 PM, Eliot Miranda <[hidden email]>
> wrote:
>
>> Stupid me.  It's got to be the DeflateStream, and its subclasses
>> ZipWriteStream, GZipWriteStream and ZLibWriteStream.  So my change
>> screws
>> all uses of these in plugins.  So what to do about fixing this?  I would
>> like to have a go at fixing the plugins.  But cheaper might be to fix
>> WriteStream with some property hack, storing the initialPosition
>> somewhere
>> else (a class side weak dictionary of stream -> initialPosition ??
>> (yuck)).
>>
>>
>> On Fri, Jul 18, 2014 at 12:31 PM, Eliot Miranda
>> <[hidden email]>
>> wrote:
>>
>>>
>>>
>>>
>>> On Thu, Jul 17, 2014 at 4:02 PM, David T. Lewis <[hidden email]>
>>> wrote:
>>>
>>>> On Fri, Jul 18, 2014 at 12:24:29AM +0200, Nicolas Cellier wrote:
>>>> > 2014-07-13 16:22 GMT+02:00 David T. Lewis <[hidden email]>:
>>>> >
>>>> > > On Sun, Jul 13, 2014 at 09:55:41AM -0400, David T. Lewis wrote:
>>>> > > > On Sun, Jul 06, 2014 at 12:47:25PM -0400, David T. Lewis wrote:
>>>> > > > > On Sun, Jul 06, 2014 at 12:23:11PM -0400, David T. Lewis
>>>> wrote:
>>>> > > > > > On Sun, Jul 06, 2014 at 10:34:15AM +0200, Nicolas Cellier
>>>> wrote:
>>>> > > > > > > 2014-07-04 0:47 GMT+02:00 Nicolas Cellier <
>>>> > > > > > > [hidden email]>:
>>>> > > > > > >
>>>> > > > > > > >
>>>> > > > > > > >
>>>> > > > > > > >
>>>> > > > > > > > 2014-07-04 0:09 GMT+02:00 Eliot Miranda <
>>>> [hidden email]
>>>> > > >:
>>>> > > > > > > >
>>>> > > > > > > >
>>>> > > > > > > >>
>>>> > > > > > > >>
>>>> > > > > > > >> On Thu, Jul 3, 2014 at 12:33 PM, Nicolas Cellier <
>>>> > > > > > > >> [hidden email]> wrote:
>>>> > > > > > > >>
>>>> > > > > > > >>> The bug is repeatable, i simply have to execute this
>>>> snippet
>>>> > > with my
>>>> > > > > > > >>> test file:
>>>> > > > > > > >>>
>>>> > > > > > > >>> (FileStream fileNamed: 'snapshot.bin') binary
>>>> > > contentsOfEntireFile
>>>> > > > > > > >>> asString zipped.
>>>> > > > > > > >>>
>>>> > > > > > > >>> The file is too big for an attachment here - 7.3
>>>> Mbytes
>>>> - or
>>>> > > 1.7 Mbytes
>>>> > > > > > > >>> gzipped by external tool.
>>>> > > > > > > >>> If someone can suggest an upload site, or want it by
>>>> mail,
>>>> > > just ask.
>>>> > > > > > > >>>
>>>> > > > > > > >>
>>>> > > > > > > >> You're welcome to put it on ftp.mirandanamda.org,
>>>> cogftpuser,
>>>> > > pw cogging
>>>> > > > > > > >> with 0's & 1's.
>>>> > > > > > > >>
>>>> > > > > > > >>
>>>> > > > > > > > done, pollution of your site engaged...
>>>> > > > > > > > Thanks Eliot!
>>>> > > > > > > >
>>>> > > > > > > >
>>>> > > > > > > >>
>>>> > > > > > > I inquired a bit more about this bug.
>>>> > > > > > > An important clue is that it does not happen in Pharo3.0!
>>>> > > > > > >
>>>> > > > > > > But Pharo3.0 did not fundamentally change compression
>>>> > > > > > > (except some FileSystem related changes, separation of CRC
>>>> stuff
>>>> > > in another
>>>> > > > > > > package, some other cosmetic changes like replacing some
>>>> self
>>>> > > assert: ...
>>>> > > > > > > by [...] assert and a potential bug in
>>>> > > > > > > DeflateStream>>nextPutAll:startingAt: introduced by
>>>> CamilleTeruel).
>>>> > > > > > > Squeak behavior is presumably not due to a Compression
>>>> change.
>>>> > > > > > >
>>>> > > > > > > Neither is it a VM problem, the bugs still shows up when
>>>> running
>>>> > > the Squeak
>>>> > > > > > > image with Pharo VM.
>>>> > > > > > >
>>>> > > > > >
>>>> > > > > > Yes, it is definite a problem in the Squeak trunk image.
>>>> > > > > >
>>>> > > > > >
>>>> > > > > > > So the difference lies somewhere else: in our beloved
>>>> Stream.
>>>> > > > > > > I remembered a recent change of Eliot related to handling
>>>> of
>>>> > > Stream created
>>>> > > > > > > with on:from:to: (Collections-eem.567).
>>>> > > > > > > Reverting those changes just make the snippet pass!
>>>> > > > > > >
>>>> > > > > > > Ah ah! At the time, i found the change of Eliot quite
>>>> minimal and
>>>> > > nice.
>>>> > > > > > > But I wish I had time to analyze this small innocent
>>>> change
>>>> deeper.
>>>> > > > > > > Indeed, I remembered I previously broke my teeth on this
>>>> one
>>>> 2
>>>> > > years or so
>>>> > > > > > > ago, and preferred to adopt another strategy: avoid using
>>>> > > on:from:to: and
>>>> > > > > > > ReadWriteStream as much as possible.
>>>> > > > > > > Why? because when analyzing the usage of inst.var. in
>>>> Stream
>>>> > > hierarchy, it
>>>> > > > > > > gave me headaches, some subclass are just ignoring
>>>> superclass
>>>> > > variables, or
>>>> > > > > > > worse are bending the semantics of superclass variables.
>>>> > > > > > > I came to the conclusion that I could not decently master
>>>> a
>>>> change
>>>> > > of
>>>> > > > > > > on:from:to:
>>>> > > > > >
>>>> > > > > > I can not confirm this. I have an image in which I can
>>>> reliably
>>>> > > reproduce the
>>>> > > > > > failure in writing the MCZ to a file. I tried reverting the
>>>> methods
>>>> > > that were
>>>> > > > > > introduced in Collections-eem.567 and I am still getting the
>>>> same
>>>> > > failure.
>>>> > > > > >
>>>> > > > > > Dave
>>>> > > > >
>>>> > > > > Oops, I spoke too soon. Indeed the problem appears as of
>>>> > > Collections-eem.567,
>>>> > > > > and does not appear in Collections-nice.566.
>>>> > > > >
>>>> > > > > So I am confirming Nicolas' observations ... now to find the
>>>> bug
>>>> :)
>>>> > > > >
>>>> > > > > Dave
>>>> > > > >
>>>> > > >
>>>> > > > This seems to be a rather tricky bug, and I don't think any of
>>>> us
>>>> knows
>>>> > > the
>>>> > > > cause right now. In case anyone else wants to have a look at it,
>>>> here is
>>>> > > a
>>>> > > > recipe for reproducing the bug in a clean 4.5 image:
>>>> > > >
>>>> > > > - Start with a clean Squeak 4.5 image in and empty working
>>>> directory.
>>>> > > >       ftp://ftp.squeak.org/4.5/Squeak4.5-13680.zip
>>>> > > >
>>>> > > > - Open the image, do not update it.
>>>> > > >
>>>> > > > - Open an MC browser and add the http://source.squeak.org/trunk
>>>> > > repository for the Help-Squeak-Project package and Collections
>>>> package.
>>>> > > >
>>>> > > > - Load Help-Squeak-Project-kfr.18 from trunk.
>>>> > > >
>>>> > > > - Load Collections-eem.567 from trunk.
>>>> > > >
>>>> > > > - Open a browser on class SqueakToolsDebuggerHelp, and delete
>>>> the
>>>> class
>>>> > > side method #showDebuggerMenuForm.
>>>> > > >
>>>> > > > - Remove the http://source.squeak.org/trunk repository form the
>>>> > > Help-Squeak-Project package in the MC browser.
>>>> > > >
>>>> > > > - Highlight your package-cache repository and try to save
>>>> > > Help-Squeak-project to the package-cache repository.
>>>> > > >
>>>> > > > - Enter author initials 'dtl'.
>>>> > > >
>>>> > > > - For the package comment, enter 'Remove unreferenced method'.
>>>> > > >
>>>> > > > - Package save will fail part way through saving the MCZ.
>>>> > > >
>>>> > > > - To reproduce the failure in this image, do the following in a
>>>> > > workspace:
>>>> > > >
>>>> > > >       mcv := MCVersion allInstances detect: [:e | e name = 'a
>>>> > > MCVersion(Help-Squeak-Project-dtl.19)'].
>>>> > > >       [f := FileStream fileNamed: 'junk.mcz'.
>>>> > > >       mcv fileOutOn: f]
>>>> > > >               ensure: [f close].
>>>> > > >
>>>> > >
>>>> > > In addition to the above recipe, here are some other observations:
>>>> > >
>>>> > > - Reverting from Collections-eem.567 to Collections-nice.566 makes
>>>> the
>>>> > >   problem go away, but ONLY if the DeflatePlugin is active.
>>>> > >
>>>> > > - The failure appears while executing the fallback code for
>>>> > > #primitiveDeflateBlock
>>>> > >   in ZipWriteStream>>deflateBlock:chainLength:goodMatch:
>>>> > >
>>>> > > - If this primitive is deactivate by commenting out the
>>>> > >   <primitive: 'primitiveDeflateBlock' module: 'ZipPlugin'> in
>>>> > >   ZipWriteStream>>deflateBlock:chainLength:goodMatch: then the
>>>> problem will
>>>> > >   occur regardless of which version of Collections is loaded.
>>>> > >
>>>> > > - When Collections-eem.567 is loaded, the primitive will fail on
>>>> the
>>>> 22nd
>>>> > >   call to #deflateBlock:chainLength:goodMatch: and a debugger
>>>> appears
>>>> > > during
>>>> > >   execution of the fallback code.
>>>> > >
>>>> > > - When Collections-nice.566 is loaded, the primitive does not fail
>>>> on the
>>>> > > 22nd
>>>> > >   call to #deflateBlock:chainLength:goodMatch:, the fallback code
>>>> is
>>>> never
>>>> > > run,
>>>> > >   and no error appears.
>>>> > >
>>>> > > - When Collections-nice.566 is loaded and the primitive is
>>>> disabled
>>>> to
>>>> > > force
>>>> > >   use of the fallback code, the error appears on on the 22nd call
>>>> to
>>>> > >   #deflateBlock:chainLength:goodMatch: in exactlly the same place
>>>> as
>>>> when
>>>> > >   Collections-eem.567 is loaded.
>>>> > >
>>>> > > - If I inspect the ZipWriteStream at the failure point (when the
>>>> debugger
>>>> > > pops
>>>> > >   up after the problem), I see no obvious difference in the state
>>>> of
>>>> the
>>>> > > stream
>>>> > >   between the Collections-eem.567 case versus the
>>>> Collections-nice.566 with
>>>> > >   primitive disabled case.
>>>> > >
>>>> > > Dave
>>>> > >
>>>> > >
>>>> > Very good mining work!
>>>> > When I hear  Stream, I think fluid...
>>>> > But these snags make it so viscous, it's like our Stream is going to
>>>> freeze
>>>> > soon.
>>>> > Or is it more than frozen? The vein you're after sounds as hard as
>>>> diamond,
>>>> > we gonna need a sharp peak pickaxe.
>>>> >
>>>>
>>>> I keep hoping that your stream fixes will somehow make this problem go
>>>> away, but
>>>> maybe we are not so lucky.
>>>>
>>>> Whenever I get more time to play with this, I think I will try to
>>>> catch
>>>> it with
>>>> gdb in the primitive. There is something strange happening that seems
>>>> to
>>>> first
>>>> be detectable in the primitive failure, but only if Eliot's image fix
>>>> is
>>>> present.
>>>>
>>>> This kind of problem is a nice way to spend the morning with a good
>>>> cup
>>>> of coffee,
>>>> as some people do with the NY Times crossword puzzle ;)
>>>>
>>>>    http://en.wikipedia.org/wiki/The_New_York_Times_crossword_puzzle
>>>>
>>>> Another "morning coffee" project is to figure out which version of the
>>>> LargeIntegersPlugin
>>>> should be used, which of course was the original topic that started
>>>> this
>>>> thread.
>>>>
>>>
>>> So given that the change adds an instance variable to WriteStream could
>>> that affect some plugin that somehow accesses a stream?  What plugin
>>> could
>>> that be?  I don't see any obvious inst vars in the DeflatePlugin or
>>> InflatePlugin.  To remind ourselves, the three changes are
>>>
>>> add inst var to WriteStream:
>>>
>>> PositionableStream subclass: #WriteStream
>>> instanceVariableNames: 'writeLimit initialPositionOrNil'
>>>  classVariableNames: ''
>>> poolDictionaries: ''
>>> category: 'Collections-Streams'
>>>
>>> use it in two places:
>>> contents
>>> "Answer with a copy of my collection from the start to the current
>>> position."
>>>
>>> readLimit := readLimit max: position.
>>> ^collection copyFrom: (initialPositionOrNil ifNil: [1]) to: position
>>>
>>> on: aCollection from: firstIndex to: lastIndex
>>>
>>> | len |
>>> collection := aCollection.
>>>  readLimit :=
>>> writeLimit := lastIndex > (len := collection size)
>>> ifTrue: [len]
>>>  ifFalse: [lastIndex].
>>> position := firstIndex <= 1
>>> ifTrue: [0]
>>>  ifFalse: [firstIndex - 1].
>>> initialPositionOrNil := position + 1
>>>
>>> What I've just found is that if I revert all three changes to
>>> WriteStream
>>> the bug goes away, but if I only revert the two method changes the bug
>>> remains.  So I think the problem is merely the adding of the inst var.
>>>  This could break some plugin which was expecting inst vars in
>>> subclasses
>>> of WriteStream to be at particular offsets determined by WriteStream
>>> instSize = 4 now being 5.  The question is which plugin(s)?
>>>
>>
> Doh!
>
> DeflatePlugin>>loadDeflateStreamFrom: rcvr
> | oop |
> <inline: false>
> ((interpreterProxy isPointers: rcvr)
>  and: [(interpreterProxy slotSizeOf: rcvr) >= 15]) ifFalse:
> [^false].
> oop := interpreterProxy fetchPointer: 0 ofObject: rcvr.
> (interpreterProxy isBytes: oop) ifFalse:
> [^false].
> zipCollection := interpreterProxy firstIndexableField: oop.
> zipCollectionSize := interpreterProxy byteSizeOf: oop.
>
> zipPosition := interpreterProxy fetchInteger: 1 ofObject: rcvr.
> zipReadLimit := interpreterProxy fetchInteger: 2 ofObject: rcvr.
> "zipWriteLimit := interpreterProxy fetchInteger: 3 ofObject: rcvr."
>
> oop := interpreterProxy fetchPointer: 4 ofObject: rcvr.
> ((interpreterProxy isWords: oop)
>  and: [(interpreterProxy slotSizeOf: oop) = DeflateHashTableSize])
> ifFalse:
> [^false].
> zipHashHead := interpreterProxy firstIndexableField: oop.
> oop := interpreterProxy fetchPointer: 5 ofObject: rcvr.
> ((interpreterProxy isWords: oop)
>  and: [(interpreterProxy slotSizeOf: oop) = DeflateWindowSize]) ifFalse:
> [^false].
> zipHashTail := interpreterProxy firstIndexableField: oop.
> zipHashValue := interpreterProxy fetchInteger: 6 ofObject: rcvr.
> zipBlockPos := interpreterProxy fetchInteger: 7 ofObject: rcvr.
> "zipBlockStart := interpreterProxy fetchInteger: 8 ofObject: rcvr."
> oop := interpreterProxy fetchPointer: 9 ofObject: rcvr.
> (interpreterProxy isBytes: oop) ifFalse:
> [^false].
> zipLiteralSize := interpreterProxy slotSizeOf: oop.
> zipLiterals := interpreterProxy firstIndexableField: oop.
>
> oop := interpreterProxy fetchPointer: 10 ofObject: rcvr.
> ((interpreterProxy isWords: oop)
>  and: [(interpreterProxy slotSizeOf: oop) >= zipLiteralSize]) ifFalse:
> [^false].
> zipDistances := interpreterProxy firstIndexableField: oop.
>
> oop := interpreterProxy fetchPointer: 11 ofObject: rcvr.
> ((interpreterProxy isWords: oop)
>  and: [(interpreterProxy slotSizeOf: oop) = DeflateMaxLiteralCodes])
> ifFalse:
> [^false].
> zipLiteralFreq := interpreterProxy firstIndexableField: oop.
>
> oop := interpreterProxy fetchPointer: 12 ofObject: rcvr.
> ((interpreterProxy isWords: oop)
>  and: [(interpreterProxy slotSizeOf: oop) = DeflateMaxDistanceCodes])
> ifFalse:
> [^false].
> zipDistanceFreq := interpreterProxy firstIndexableField: oop.
>
> zipLiteralCount := interpreterProxy fetchInteger: 13 ofObject: rcvr.
> zipMatchCount := interpreterProxy fetchInteger: 14 ofObject: rcvr.
>
> ^interpreterProxy failed not
>
> I propose to add a variable that holds the inst size of the superclass of
> InflateStream, and to use this to correct the offsets.  Objections?
> --

I can't look at the code right now, but any fix needs to be done in the
image, otherwise all existing VMs are broken. Can we arrange for the added
instance variable to be the last instance variable such that it does not
effect existing references from the plugins (suitable commented of
course)? Please disregard if I am misunderstanding, I don't have an image
to look at.

Note that the primitive fallback code is probably still broken, but
perhaps that's a separate issue entirely.

Dave


Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] Re: [Vm-dev] Bug in writing compressed stream when saving an mcz (was: New Cog VMs available...)

Eliot Miranda-2
 
Hi David,


On Fri, Jul 18, 2014 at 1:51 PM, David T. Lewis <[hidden email]> wrote:

>  On Fri, Jul 18, 2014 at 12:38 PM, Eliot Miranda <[hidden email]>
> wrote:
>
>> Stupid me.  It's got to be the DeflateStream, and its subclasses
>> ZipWriteStream, GZipWriteStream and ZLibWriteStream.  So my change
>> screws
>> all uses of these in plugins.  So what to do about fixing this?  I would
>> like to have a go at fixing the plugins.  But cheaper might be to fix
>> WriteStream with some property hack, storing the initialPosition
>> somewhere
>> else (a class side weak dictionary of stream -> initialPosition ??
>> (yuck)).
>>
>>
>> On Fri, Jul 18, 2014 at 12:31 PM, Eliot Miranda
>> <[hidden email]>
>> wrote:
>>
>>>
>>>
>>>
>>> On Thu, Jul 17, 2014 at 4:02 PM, David T. Lewis <[hidden email]>
>>> wrote:
>>>
>>>> On Fri, Jul 18, 2014 at 12:24:29AM +0200, Nicolas Cellier wrote:
>>>> > 2014-07-13 16:22 GMT+02:00 David T. Lewis <[hidden email]>:
>>>> >
>>>> > > On Sun, Jul 13, 2014 at 09:55:41AM -0400, David T. Lewis wrote:
>>>> > > > On Sun, Jul 06, 2014 at 12:47:25PM -0400, David T. Lewis wrote:
>>>> > > > > On Sun, Jul 06, 2014 at 12:23:11PM -0400, David T. Lewis
>>>> wrote:
>>>> > > > > > On Sun, Jul 06, 2014 at 10:34:15AM +0200, Nicolas Cellier
>>>> wrote:
>>>> > > > > > > 2014-07-04 0:47 GMT+02:00 Nicolas Cellier <
>>>> > > > > > > [hidden email]>:
>>>> > > > > > >
>>>> > > > > > > >
>>>> > > > > > > >
>>>> > > > > > > >
>>>> > > > > > > > 2014-07-04 0:09 GMT+02:00 Eliot Miranda <
>>>> [hidden email]
>>>> > > >:
>>>> > > > > > > >
>>>> > > > > > > >
>>>> > > > > > > >>
>>>> > > > > > > >>
>>>> > > > > > > >> On Thu, Jul 3, 2014 at 12:33 PM, Nicolas Cellier <
>>>> > > > > > > >> [hidden email]> wrote:
>>>> > > > > > > >>
>>>> > > > > > > >>> The bug is repeatable, i simply have to execute this
>>>> snippet
>>>> > > with my
>>>> > > > > > > >>> test file:
>>>> > > > > > > >>>
>>>> > > > > > > >>> (FileStream fileNamed: 'snapshot.bin') binary
>>>> > > contentsOfEntireFile
>>>> > > > > > > >>> asString zipped.
>>>> > > > > > > >>>
>>>> > > > > > > >>> The file is too big for an attachment here - 7.3
>>>> Mbytes
>>>> - or
>>>> > > 1.7 Mbytes
>>>> > > > > > > >>> gzipped by external tool.
>>>> > > > > > > >>> If someone can suggest an upload site, or want it by
>>>> mail,
>>>> > > just ask.
>>>> > > > > > > >>>
>>>> > > > > > > >>
>>>> > > > > > > >> You're welcome to put it on ftp.mirandanamda.org,
>>>> cogftpuser,
>>>> > > pw cogging
>>>> > > > > > > >> with 0's & 1's.
>>>> > > > > > > >>
>>>> > > > > > > >>
>>>> > > > > > > > done, pollution of your site engaged...
>>>> > > > > > > > Thanks Eliot!
>>>> > > > > > > >
>>>> > > > > > > >
>>>> > > > > > > >>
>>>> > > > > > > I inquired a bit more about this bug.
>>>> > > > > > > An important clue is that it does not happen in Pharo3.0!
>>>> > > > > > >
>>>> > > > > > > But Pharo3.0 did not fundamentally change compression
>>>> > > > > > > (except some FileSystem related changes, separation of CRC
>>>> stuff
>>>> > > in another
>>>> > > > > > > package, some other cosmetic changes like replacing some
>>>> self
>>>> > > assert: ...
>>>> > > > > > > by [...] assert and a potential bug in
>>>> > > > > > > DeflateStream>>nextPutAll:startingAt: introduced by
>>>> CamilleTeruel).
>>>> > > > > > > Squeak behavior is presumably not due to a Compression
>>>> change.
>>>> > > > > > >
>>>> > > > > > > Neither is it a VM problem, the bugs still shows up when
>>>> running
>>>> > > the Squeak
>>>> > > > > > > image with Pharo VM.
>>>> > > > > > >
>>>> > > > > >
>>>> > > > > > Yes, it is definite a problem in the Squeak trunk image.
>>>> > > > > >
>>>> > > > > >
>>>> > > > > > > So the difference lies somewhere else: in our beloved
>>>> Stream.
>>>> > > > > > > I remembered a recent change of Eliot related to handling
>>>> of
>>>> > > Stream created
>>>> > > > > > > with on:from:to: (Collections-eem.567).
>>>> > > > > > > Reverting those changes just make the snippet pass!
>>>> > > > > > >
>>>> > > > > > > Ah ah! At the time, i found the change of Eliot quite
>>>> minimal and
>>>> > > nice.
>>>> > > > > > > But I wish I had time to analyze this small innocent
>>>> change
>>>> deeper.
>>>> > > > > > > Indeed, I remembered I previously broke my teeth on this
>>>> one
>>>> 2
>>>> > > years or so
>>>> > > > > > > ago, and preferred to adopt another strategy: avoid using
>>>> > > on:from:to: and
>>>> > > > > > > ReadWriteStream as much as possible.
>>>> > > > > > > Why? because when analyzing the usage of inst.var. in
>>>> Stream
>>>> > > hierarchy, it
>>>> > > > > > > gave me headaches, some subclass are just ignoring
>>>> superclass
>>>> > > variables, or
>>>> > > > > > > worse are bending the semantics of superclass variables.
>>>> > > > > > > I came to the conclusion that I could not decently master
>>>> a
>>>> change
>>>> > > of
>>>> > > > > > > on:from:to:
>>>> > > > > >
>>>> > > > > > I can not confirm this. I have an image in which I can
>>>> reliably
>>>> > > reproduce the
>>>> > > > > > failure in writing the MCZ to a file. I tried reverting the
>>>> methods
>>>> > > that were
>>>> > > > > > introduced in Collections-eem.567 and I am still getting the
>>>> same
>>>> > > failure.
>>>> > > > > >
>>>> > > > > > Dave
>>>> > > > >
>>>> > > > > Oops, I spoke too soon. Indeed the problem appears as of
>>>> > > Collections-eem.567,
>>>> > > > > and does not appear in Collections-nice.566.
>>>> > > > >
>>>> > > > > So I am confirming Nicolas' observations ... now to find the
>>>> bug
>>>> :)
>>>> > > > >
>>>> > > > > Dave
>>>> > > > >
>>>> > > >
>>>> > > > This seems to be a rather tricky bug, and I don't think any of
>>>> us
>>>> knows
>>>> > > the
>>>> > > > cause right now. In case anyone else wants to have a look at it,
>>>> here is
>>>> > > a
>>>> > > > recipe for reproducing the bug in a clean 4.5 image:
>>>> > > >
>>>> > > > - Start with a clean Squeak 4.5 image in and empty working
>>>> directory.
>>>> > > >       ftp://ftp.squeak.org/4.5/Squeak4.5-13680.zip
>>>> > > >
>>>> > > > - Open the image, do not update it.
>>>> > > >
>>>> > > > - Open an MC browser and add the http://source.squeak.org/trunk
>>>> > > repository for the Help-Squeak-Project package and Collections
>>>> package.
>>>> > > >
>>>> > > > - Load Help-Squeak-Project-kfr.18 from trunk.
>>>> > > >
>>>> > > > - Load Collections-eem.567 from trunk.
>>>> > > >
>>>> > > > - Open a browser on class SqueakToolsDebuggerHelp, and delete
>>>> the
>>>> class
>>>> > > side method #showDebuggerMenuForm.
>>>> > > >
>>>> > > > - Remove the http://source.squeak.org/trunk repository form the
>>>> > > Help-Squeak-Project package in the MC browser.
>>>> > > >
>>>> > > > - Highlight your package-cache repository and try to save
>>>> > > Help-Squeak-project to the package-cache repository.
>>>> > > >
>>>> > > > - Enter author initials 'dtl'.
>>>> > > >
>>>> > > > - For the package comment, enter 'Remove unreferenced method'.
>>>> > > >
>>>> > > > - Package save will fail part way through saving the MCZ.
>>>> > > >
>>>> > > > - To reproduce the failure in this image, do the following in a
>>>> > > workspace:
>>>> > > >
>>>> > > >       mcv := MCVersion allInstances detect: [:e | e name = 'a
>>>> > > MCVersion(Help-Squeak-Project-dtl.19)'].
>>>> > > >       [f := FileStream fileNamed: 'junk.mcz'.
>>>> > > >       mcv fileOutOn: f]
>>>> > > >               ensure: [f close].
>>>> > > >
>>>> > >
>>>> > > In addition to the above recipe, here are some other observations:
>>>> > >
>>>> > > - Reverting from Collections-eem.567 to Collections-nice.566 makes
>>>> the
>>>> > >   problem go away, but ONLY if the DeflatePlugin is active.
>>>> > >
>>>> > > - The failure appears while executing the fallback code for
>>>> > > #primitiveDeflateBlock
>>>> > >   in ZipWriteStream>>deflateBlock:chainLength:goodMatch:
>>>> > >
>>>> > > - If this primitive is deactivate by commenting out the
>>>> > >   <primitive: 'primitiveDeflateBlock' module: 'ZipPlugin'> in
>>>> > >   ZipWriteStream>>deflateBlock:chainLength:goodMatch: then the
>>>> problem will
>>>> > >   occur regardless of which version of Collections is loaded.
>>>> > >
>>>> > > - When Collections-eem.567 is loaded, the primitive will fail on
>>>> the
>>>> 22nd
>>>> > >   call to #deflateBlock:chainLength:goodMatch: and a debugger
>>>> appears
>>>> > > during
>>>> > >   execution of the fallback code.
>>>> > >
>>>> > > - When Collections-nice.566 is loaded, the primitive does not fail
>>>> on the
>>>> > > 22nd
>>>> > >   call to #deflateBlock:chainLength:goodMatch:, the fallback code
>>>> is
>>>> never
>>>> > > run,
>>>> > >   and no error appears.
>>>> > >
>>>> > > - When Collections-nice.566 is loaded and the primitive is
>>>> disabled
>>>> to
>>>> > > force
>>>> > >   use of the fallback code, the error appears on on the 22nd call
>>>> to
>>>> > >   #deflateBlock:chainLength:goodMatch: in exactlly the same place
>>>> as
>>>> when
>>>> > >   Collections-eem.567 is loaded.
>>>> > >
>>>> > > - If I inspect the ZipWriteStream at the failure point (when the
>>>> debugger
>>>> > > pops
>>>> > >   up after the problem), I see no obvious difference in the state
>>>> of
>>>> the
>>>> > > stream
>>>> > >   between the Collections-eem.567 case versus the
>>>> Collections-nice.566 with
>>>> > >   primitive disabled case.
>>>> > >
>>>> > > Dave
>>>> > >
>>>> > >
>>>> > Very good mining work!
>>>> > When I hear  Stream, I think fluid...
>>>> > But these snags make it so viscous, it's like our Stream is going to
>>>> freeze
>>>> > soon.
>>>> > Or is it more than frozen? The vein you're after sounds as hard as
>>>> diamond,
>>>> > we gonna need a sharp peak pickaxe.
>>>> >
>>>>
>>>> I keep hoping that your stream fixes will somehow make this problem go
>>>> away, but
>>>> maybe we are not so lucky.
>>>>
>>>> Whenever I get more time to play with this, I think I will try to
>>>> catch
>>>> it with
>>>> gdb in the primitive. There is something strange happening that seems
>>>> to
>>>> first
>>>> be detectable in the primitive failure, but only if Eliot's image fix
>>>> is
>>>> present.
>>>>
>>>> This kind of problem is a nice way to spend the morning with a good
>>>> cup
>>>> of coffee,
>>>> as some people do with the NY Times crossword puzzle ;)
>>>>
>>>>    http://en.wikipedia.org/wiki/The_New_York_Times_crossword_puzzle
>>>>
>>>> Another "morning coffee" project is to figure out which version of the
>>>> LargeIntegersPlugin
>>>> should be used, which of course was the original topic that started
>>>> this
>>>> thread.
>>>>
>>>
>>> So given that the change adds an instance variable to WriteStream could
>>> that affect some plugin that somehow accesses a stream?  What plugin
>>> could
>>> that be?  I don't see any obvious inst vars in the DeflatePlugin or
>>> InflatePlugin.  To remind ourselves, the three changes are
>>>
>>> add inst var to WriteStream:
>>>
>>> PositionableStream subclass: #WriteStream
>>> instanceVariableNames: 'writeLimit initialPositionOrNil'
>>>  classVariableNames: ''
>>> poolDictionaries: ''
>>> category: 'Collections-Streams'
>>>
>>> use it in two places:
>>> contents
>>> "Answer with a copy of my collection from the start to the current
>>> position."
>>>
>>> readLimit := readLimit max: position.
>>> ^collection copyFrom: (initialPositionOrNil ifNil: [1]) to: position
>>>
>>> on: aCollection from: firstIndex to: lastIndex
>>>
>>> | len |
>>> collection := aCollection.
>>>  readLimit :=
>>> writeLimit := lastIndex > (len := collection size)
>>> ifTrue: [len]
>>>  ifFalse: [lastIndex].
>>> position := firstIndex <= 1
>>> ifTrue: [0]
>>>  ifFalse: [firstIndex - 1].
>>> initialPositionOrNil := position + 1
>>>
>>> What I've just found is that if I revert all three changes to
>>> WriteStream
>>> the bug goes away, but if I only revert the two method changes the bug
>>> remains.  So I think the problem is merely the adding of the inst var.
>>>  This could break some plugin which was expecting inst vars in
>>> subclasses
>>> of WriteStream to be at particular offsets determined by WriteStream
>>> instSize = 4 now being 5.  The question is which plugin(s)?
>>>
>>
> Doh!
>
> DeflatePlugin>>loadDeflateStreamFrom: rcvr
> | oop |
> <inline: false>
> ((interpreterProxy isPointers: rcvr)
>  and: [(interpreterProxy slotSizeOf: rcvr) >= 15]) ifFalse:
> [^false].
> oop := interpreterProxy fetchPointer: 0 ofObject: rcvr.
> (interpreterProxy isBytes: oop) ifFalse:
> [^false].
> zipCollection := interpreterProxy firstIndexableField: oop.
> zipCollectionSize := interpreterProxy byteSizeOf: oop.
>
> zipPosition := interpreterProxy fetchInteger: 1 ofObject: rcvr.
> zipReadLimit := interpreterProxy fetchInteger: 2 ofObject: rcvr.
> "zipWriteLimit := interpreterProxy fetchInteger: 3 ofObject: rcvr."
>
> oop := interpreterProxy fetchPointer: 4 ofObject: rcvr.
> ((interpreterProxy isWords: oop)
>  and: [(interpreterProxy slotSizeOf: oop) = DeflateHashTableSize])
> ifFalse:
> [^false].
> zipHashHead := interpreterProxy firstIndexableField: oop.
> oop := interpreterProxy fetchPointer: 5 ofObject: rcvr.
> ((interpreterProxy isWords: oop)
>  and: [(interpreterProxy slotSizeOf: oop) = DeflateWindowSize]) ifFalse:
> [^false].
> zipHashTail := interpreterProxy firstIndexableField: oop.
> zipHashValue := interpreterProxy fetchInteger: 6 ofObject: rcvr.
> zipBlockPos := interpreterProxy fetchInteger: 7 ofObject: rcvr.
> "zipBlockStart := interpreterProxy fetchInteger: 8 ofObject: rcvr."
> oop := interpreterProxy fetchPointer: 9 ofObject: rcvr.
> (interpreterProxy isBytes: oop) ifFalse:
> [^false].
> zipLiteralSize := interpreterProxy slotSizeOf: oop.
> zipLiterals := interpreterProxy firstIndexableField: oop.
>
> oop := interpreterProxy fetchPointer: 10 ofObject: rcvr.
> ((interpreterProxy isWords: oop)
>  and: [(interpreterProxy slotSizeOf: oop) >= zipLiteralSize]) ifFalse:
> [^false].
> zipDistances := interpreterProxy firstIndexableField: oop.
>
> oop := interpreterProxy fetchPointer: 11 ofObject: rcvr.
> ((interpreterProxy isWords: oop)
>  and: [(interpreterProxy slotSizeOf: oop) = DeflateMaxLiteralCodes])
> ifFalse:
> [^false].
> zipLiteralFreq := interpreterProxy firstIndexableField: oop.
>
> oop := interpreterProxy fetchPointer: 12 ofObject: rcvr.
> ((interpreterProxy isWords: oop)
>  and: [(interpreterProxy slotSizeOf: oop) = DeflateMaxDistanceCodes])
> ifFalse:
> [^false].
> zipDistanceFreq := interpreterProxy firstIndexableField: oop.
>
> zipLiteralCount := interpreterProxy fetchInteger: 13 ofObject: rcvr.
> zipMatchCount := interpreterProxy fetchInteger: 14 ofObject: rcvr.
>
> ^interpreterProxy failed not
>
> I propose to add a variable that holds the inst size of the superclass of
> InflateStream, and to use this to correct the offsets.  Objections?
> --

I can't look at the code right now, but any fix needs to be done in the
image, otherwise all existing VMs are broken.

I think we have to accept that the fix to on:from:to: means that exsting VMs are broken.  Certainly the ZipPlugin (InflatePlugin & DeflatePlugin)'s dependency on WriteStream and ReadStream inst size is restrictive.  I'd rather lift the restriction and use new VMs than live with the restriction long-term.

 
Can we arrange for the added
instance variable to be the last instance variable such that it does not
effect existing references from the plugins (suitable commented of
course)? Please disregard if I am misunderstanding, I don't have an image
to look at.

The issue is that the inst var added (initialPositionOrNil) is to ReadStream and WriteStream and hence affects the inst var offsets in all subclasses, and these include DeflateStream, ZipEncoder and InflateStream, which the ZipPlugin accesses.

Note that the primitive fallback code is probably still broken, but
perhaps that's a separate issue entirely.
 
I'm not convinced yet.  Certainly if the primitives are all removed and the image still fails then the fall;back code is broken.  But we need to determine that carefully. 
--
best,
Eliot
Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] Re: [Vm-dev] Bug in writing compressed stream when saving an mcz (was: New Cog VMs available...)

Eliot Miranda-2
In reply to this post by Eliot Miranda-2
 
Hi All,

    I'm close to a fix on this, but I have to stop as I've some errands to run.  I wonder if anyone is interested in reviewing the code.  There's a mistake in there somewhere.  Careful review will help.

The issue is to match the inst var offsets in the InflatePlugin and DeflatePlugin methods with those in InflateStream DeflateStream ZipWriteStream and ZipEncoder now that the plugin code uses readStreamInstSize and writeStreamInstSize to get at them.  Perhaps some kind soul can read this code like a lawyer and spot my mistake?  I'll try and take another look at this this evening.

Changes attached and included for your perusal:

'From Squeak4.5 of 18 July 2014 [latest update: #13848] on 18 July 2014 at 2:34:04 pm'!
InflatePlugin subclass: #DeflatePlugin
instanceVariableNames: 'zipHashHead zipHashTail zipHashValue zipBlockPos zipBlockStart zipLiterals zipDistances zipLiteralFreq zipDistanceFreq zipLiteralCount zipLiteralSize zipMatchCount zipMatchLengthCodes zipDistanceCodes zipCrcTable zipExtraLengthBits zipExtraDistanceBits zipBaseLength zipBaseDistance writeStreamInstSize'
classVariableNames: 'DeflateHashBits DeflateHashMask DeflateHashShift DeflateHashTableSize DeflateMaxDistance DeflateMaxDistanceCodes DeflateMaxLiteralCodes DeflateMaxMatch DeflateMinMatch DeflateWindowMask DeflateWindowSize'
poolDictionaries: ''
category: 'VMMaker-Plugins'!
!DeflatePlugin commentStamp: 'tpr 5/5/2003 11:52' prior: 0!
This adds Zip deflating support.
InflatePlugin should not be translated but this subclass should since it is incorporated within that class's translation process!

!DeflatePlugin methodsFor: 'primitive support' stamp: 'eem 7/18/2014 13:56'!
determineSizeOfWriteStream: rcvr
"Determine the inst size of the class above DeflateStream or
ZipEncoder by looking for the first class whose inst size is less than 7."
| class |
class := interpreterProxy fetchClassOf: rcvr.
[class ~= interpreterProxy nilObject
and: [(interpreterProxy instanceSizeOf: class) >= 7]] whileTrue:
[class := interpreterProxy superclassOf: class].
class = interpreterProxy nilObject ifTrue:
[^false].
writeStreamInstSize := interpreterProxy instanceSizeOf: class.
^true
! !

!DeflatePlugin methodsFor: 'primitive support' stamp: 'eem 7/18/2014 14:15'!
loadDeflateStreamFrom: rcvr
| oop |
<inline: false>
((interpreterProxy isPointers: rcvr)
and: [(interpreterProxy slotSizeOf: rcvr) >= 15]) ifFalse:
[^false].
oop := interpreterProxy fetchPointer: 0 ofObject: rcvr.
(interpreterProxy isBytes: oop) ifFalse:
[^false].
writeStreamInstSize = 0 ifTrue:
[(self determineSizeOfWriteStream: rcvr) ifFalse:
[^false].
"If the receiver wasn't valid then we derived writeStreamInstSize from an invalid source.  discard it."
(interpreterProxy slotSizeOf: rcvr) < (writeStreamInstSize + 5) ifTrue:
[writeStreamInstSize := 0.
^false]].
zipCollection := interpreterProxy firstIndexableField: oop.
zipCollectionSize := interpreterProxy byteSizeOf: oop.

zipPosition := interpreterProxy fetchInteger: 1 ofObject: rcvr.
zipReadLimit := interpreterProxy fetchInteger: 2 ofObject: rcvr.
"zipWriteLimit := interpreterProxy fetchInteger: 3 ofObject: rcvr."

oop := interpreterProxy fetchPointer: writeStreamInstSize + 0 ofObject: rcvr.
((interpreterProxy isWords: oop)
and: [(interpreterProxy slotSizeOf: oop) = DeflateHashTableSize]) ifFalse:
[^false].
zipHashHead := interpreterProxy firstIndexableField: oop.
oop := interpreterProxy fetchPointer: writeStreamInstSize + 1 ofObject: rcvr.
((interpreterProxy isWords: oop)
and: [(interpreterProxy slotSizeOf: oop) = DeflateWindowSize]) ifFalse:
[^false].
zipHashTail := interpreterProxy firstIndexableField: oop.
zipHashValue := interpreterProxy fetchInteger: writeStreamInstSize + 2 ofObject: rcvr.
zipBlockPos := interpreterProxy fetchInteger: writeStreamInstSize + 3 ofObject: rcvr.
"zipBlockStart := interpreterProxy fetchInteger: writeStreamInstSize + 4 ofObject: rcvr."
oop := interpreterProxy fetchPointer: writeStreamInstSize + 5 ofObject: rcvr.
(interpreterProxy isBytes: oop) ifFalse:
[^false].
zipLiteralSize := interpreterProxy slotSizeOf: oop.
zipLiterals := interpreterProxy firstIndexableField: oop.

oop := interpreterProxy fetchPointer: writeStreamInstSize + 6 ofObject: rcvr.
((interpreterProxy isWords: oop)
and: [(interpreterProxy slotSizeOf: oop) >= zipLiteralSize]) ifFalse:
[^false].
zipDistances := interpreterProxy firstIndexableField: oop.

oop := interpreterProxy fetchPointer: writeStreamInstSize + 7 ofObject: rcvr.
((interpreterProxy isWords: oop)
and: [(interpreterProxy slotSizeOf: oop) = DeflateMaxLiteralCodes]) ifFalse:
[^false].
zipLiteralFreq := interpreterProxy firstIndexableField: oop.

oop := interpreterProxy fetchPointer: writeStreamInstSize + 8 ofObject: rcvr.
((interpreterProxy isWords: oop)
and: [(interpreterProxy slotSizeOf: oop) = DeflateMaxDistanceCodes]) ifFalse:
[^false].
zipDistanceFreq := interpreterProxy firstIndexableField: oop.

zipLiteralCount := interpreterProxy fetchInteger: writeStreamInstSize + 9 ofObject: rcvr.
zipMatchCount := interpreterProxy fetchInteger: writeStreamInstSize + 10 ofObject: rcvr.

^interpreterProxy failed not! !

!DeflatePlugin methodsFor: 'primitive support' stamp: 'eem 7/18/2014 14:15'!
loadZipEncoderFrom: rcvr
| oop |
<inline: false>
writeStreamInstSize = 0 ifTrue:
[(self determineSizeOfWriteStream: rcvr) ifFalse:
[^false].
"If the receiver wasn't valid then we derived writeStreamInstSize from an invalid source.  discard it."
(interpreterProxy slotSizeOf: rcvr) < (writeStreamInstSize + 3) ifTrue:
[writeStreamInstSize := 0.
^false]].
((interpreterProxy isPointers: rcvr)
and: [(interpreterProxy slotSizeOf: rcvr) >= (writeStreamInstSize + 3)]) ifFalse:
[^false].
oop := interpreterProxy fetchPointer: 0 ofObject: rcvr.
(interpreterProxy isBytes: oop) ifFalse:
[^interpreterProxy primitiveFail].
zipCollection := interpreterProxy firstIndexableField: oop.
zipCollectionSize := interpreterProxy byteSizeOf: oop.

zipPosition := interpreterProxy fetchInteger: 1 ofObject: rcvr.
zipReadLimit := interpreterProxy fetchInteger: 2 ofObject: rcvr.
"zipWriteLimit := interpreterProxy fetchInteger: 3 ofObject: rcvr."
zipBitBuf := interpreterProxy fetchInteger: writeStreamInstSize + 0 ofObject: rcvr.
zipBitPos := interpreterProxy fetchInteger: writeStreamInstSize + 1 ofObject: rcvr.

^interpreterProxy failed not! !

!DeflatePlugin methodsFor: 'primitives' stamp: 'eem 7/18/2014 14:28'!
primitiveDeflateBlock
"Primitive. Deflate the current contents of the receiver."
| goodMatch chainLength lastIndex rcvr result |
<export: true>
<inline: false>
interpreterProxy methodArgumentCount = 3
ifFalse:[^interpreterProxy primitiveFail].
goodMatch := interpreterProxy stackIntegerValue: 0.
chainLength := interpreterProxy stackIntegerValue: 1.
lastIndex := interpreterProxy stackIntegerValue: 2.
rcvr := interpreterProxy stackObjectValue: 3.
interpreterProxy failed ifTrue:[^nil].
self cCode:'' inSmalltalk:[
zipMatchLengthCodes := CArrayAccessor on: ZipWriteStream matchLengthCodes.
zipDistanceCodes := CArrayAccessor on: ZipWriteStream distanceCodes].
(self loadDeflateStreamFrom: rcvr)
ifFalse:[^interpreterProxy primitiveFail].
result := self deflateBlock: lastIndex chainLength: chainLength goodMatch: goodMatch.
interpreterProxy failed ifFalse:[
"Store back modified values"
interpreterProxy storeInteger: writeStreamInstSize + 2 ofObject: rcvr withValue: zipHashValue.
interpreterProxy storeInteger: writeStreamInstSize + 3 ofObject: rcvr withValue: zipBlockPos.
interpreterProxy storeInteger: writeStreamInstSize + 9 ofObject: rcvr withValue: zipLiteralCount.
interpreterProxy storeInteger: writeStreamInstSize + 10 ofObject: rcvr withValue: zipMatchCount].
interpreterProxy failed ifFalse:[
interpreterProxy pop: 4.
interpreterProxy pushBool: result.
].! !

!DeflatePlugin methodsFor: 'primitives' stamp: 'eem 7/18/2014 14:24'!
primitiveZipSendBlock
| distTree litTree distStream litStream rcvr result |
<export: true>
interpreterProxy methodArgumentCount = 4 
ifFalse:[^interpreterProxy primitiveFail].
distTree := interpreterProxy stackObjectValue: 0.
litTree := interpreterProxy stackObjectValue: 1.
distStream := interpreterProxy stackObjectValue: 2.
litStream := interpreterProxy stackObjectValue: 3.
rcvr := interpreterProxy stackObjectValue: 4.
interpreterProxy failed ifTrue:[^nil].
(self loadZipEncoderFrom: rcvr)
ifFalse:[^interpreterProxy primitiveFail].
((interpreterProxy isPointers: distTree) and:[
(interpreterProxy slotSizeOf: distTree) >= 2])
ifFalse:[^interpreterProxy primitiveFail].
((interpreterProxy isPointers: litTree) and:[
(interpreterProxy slotSizeOf: litTree) >= 2])
ifFalse:[^interpreterProxy primitiveFail].
((interpreterProxy isPointers: litStream) and:[
(interpreterProxy slotSizeOf: litStream) >= 3])
ifFalse:[^interpreterProxy primitiveFail].
((interpreterProxy isPointers: distStream) and:[
(interpreterProxy slotSizeOf: distStream) >= 3])
ifFalse:[^interpreterProxy primitiveFail].
self cCode:'' inSmalltalk:[
zipMatchLengthCodes := CArrayAccessor on: ZipWriteStream matchLengthCodes.
zipDistanceCodes := CArrayAccessor on: ZipWriteStream distanceCodes.
zipExtraLengthBits := CArrayAccessor on: ZipWriteStream extraLengthBits.
zipExtraDistanceBits := CArrayAccessor on: ZipWriteStream extraDistanceBits.
zipBaseLength := CArrayAccessor on: ZipWriteStream baseLength.
zipBaseDistance := CArrayAccessor on: ZipWriteStream baseDistance].
result := self sendBlock: litStream with: distStream with: litTree with: distTree.
interpreterProxy failed ifFalse:[
interpreterProxy storeInteger: 1 ofObject: rcvr withValue: zipPosition.
interpreterProxy storeInteger: readStreamInstSize + 1 ofObject: rcvr withValue: zipBitBuf.
interpreterProxy storeInteger: readStreamInstSize + 2 ofObject: rcvr withValue: zipBitPos.
].
interpreterProxy failed ifFalse:[
interpreterProxy pop: 5. "rcvr + args"
interpreterProxy pushInteger: result.
].! !
InterpreterPlugin subclass: #InflatePlugin
instanceVariableNames: 'zipCollection zipReadLimit zipPosition zipState zipBitBuf zipBitPos zipSource zipSourcePos zipSourceLimit zipLitTable zipDistTable zipCollectionSize zipLitTableSize zipDistTableSize readStreamInstSize'
classVariableNames: 'MaxBits StateNoMoreData'
poolDictionaries: ''
category: 'VMMaker-Plugins'!
!InflatePlugin commentStamp: '<historical>' prior: 0!
This plugin implements the one crucial function for efficiently decompressing streams.!

!InflatePlugin methodsFor: 'primitive support' stamp: 'eem 7/18/2014 14:02'!
determineSizeOfReadStream: rcvr
"Determine the inst size of the class above DeflateStream by
looking for the first class whose inst size is less than 14."
| class |
class := interpreterProxy fetchClassOf: rcvr.
[class ~= interpreterProxy nilObject
and: [(interpreterProxy instanceSizeOf: class) >= 14]] whileTrue:
[class := interpreterProxy superclassOf: class].
class = interpreterProxy nilObject ifTrue:
[^false].
readStreamInstSize := interpreterProxy instanceSizeOf: class.
^true! !

!InflatePlugin methodsFor: 'primitives' stamp: 'eem 7/18/2014 14:18'!
primitiveInflateDecompressBlock
"Primitive. Inflate a single block."
| oop rcvr |
<export: true>
interpreterProxy methodArgumentCount = 2 ifFalse:
[^interpreterProxy primitiveFail].
"distance table"
oop := interpreterProxy stackValue: 0.
(interpreterProxy isWords: oop) ifFalse:
[^interpreterProxy primitiveFail].
zipDistTable := interpreterProxy firstIndexableField: oop.
zipDistTableSize := interpreterProxy slotSizeOf: oop.

"literal table"
oop := interpreterProxy stackValue: 1.
(interpreterProxy isWords: oop) ifFalse:
[^interpreterProxy primitiveFail].
zipLitTable := interpreterProxy firstIndexableField: oop.
zipLitTableSize := interpreterProxy slotSizeOf: oop.


"Receiver (InflateStream)"
rcvr := interpreterProxy stackValue: 2.
(interpreterProxy isPointers: rcvr) ifFalse:
[^interpreterProxy primitiveFail].
"All the integer instvars"
readStreamInstSize = 0 ifTrue:
[(self determineSizeOfReadStream: rcvr) ifFalse:
[^interpreterProxy primitiveFail].
"If the receiver wasn't valid then we derived readStreamInstSize from an invalid source.  discard it."
(interpreterProxy slotSizeOf: rcvr) < (readStreamInstSize + 6) ifTrue:
[readStreamInstSize := 0.
^interpreterProxy primitiveFail]].
(interpreterProxy slotSizeOf: rcvr) < (readStreamInstSize + 7) ifTrue:
[^interpreterProxy primitiveFail].

zipReadLimit := interpreterProxy fetchInteger: 2 ofObject: rcvr.
zipState := interpreterProxy fetchInteger: readStreamInstSize + 0 ofObject: rcvr.
zipBitBuf := interpreterProxy fetchInteger: readStreamInstSize + 1 ofObject: rcvr.
zipBitPos := interpreterProxy fetchInteger: readStreamInstSize + 2 ofObject: rcvr.
zipSourcePos := interpreterProxy fetchInteger: readStreamInstSize + 4 ofObject: rcvr.
zipSourceLimit := interpreterProxy fetchInteger: readStreamInstSize + 5 ofObject: rcvr.
interpreterProxy failed ifTrue:[^nil].
zipReadLimit := zipReadLimit - 1.
zipSourcePos := zipSourcePos - 1.
zipSourceLimit := zipSourceLimit - 1.

"collection"
oop := interpreterProxy fetchPointer: 0 ofObject: rcvr.
(interpreterProxy isBytes: oop) ifFalse:
[^interpreterProxy primitiveFail].
zipCollection := interpreterProxy firstIndexableField: oop.
zipCollectionSize := interpreterProxy byteSizeOf: oop.

"source"
oop := interpreterProxy fetchPointer: readStreamInstSize + 3 ofObject: rcvr.
(interpreterProxy isBytes: oop) ifFalse:
[^interpreterProxy primitiveFail].
zipSource := interpreterProxy firstIndexableField: oop.

"do the primitive"
self zipDecompressBlock.
interpreterProxy failed ifFalse: "store modified values back"
[interpreterProxy storeInteger: 2 ofObject: rcvr withValue: zipReadLimit + 1.
interpreterProxy storeInteger: readStreamInstSize + 0 ofObject: rcvr withValue: zipState.
interpreterProxy storeInteger: readStreamInstSize + 1 ofObject: rcvr withValue: zipBitBuf.
interpreterProxy storeInteger: readStreamInstSize + 2 ofObject: rcvr withValue: zipBitPos.
interpreterProxy storeInteger: readStreamInstSize + 4 ofObject: rcvr withValue: zipSourcePos + 1.
interpreterProxy pop: 2]! !



On Fri, Jul 18, 2014 at 12:46 PM, Eliot Miranda <[hidden email]> wrote:



On Fri, Jul 18, 2014 at 12:38 PM, Eliot Miranda <[hidden email]> wrote:
Stupid me.  It's got to be the DeflateStream, and its subclasses ZipWriteStream, GZipWriteStream and ZLibWriteStream.  So my change screws all uses of these in plugins.  So what to do about fixing this?  I would like to have a go at fixing the plugins.  But cheaper might be to fix WriteStream with some property hack, storing the initialPosition somewhere else (a class side weak dictionary of stream -> initialPosition ?? (yuck)).


On Fri, Jul 18, 2014 at 12:31 PM, Eliot Miranda <[hidden email]> wrote:



On Thu, Jul 17, 2014 at 4:02 PM, David T. Lewis <[hidden email]> wrote:
On Fri, Jul 18, 2014 at 12:24:29AM +0200, Nicolas Cellier wrote:
> 2014-07-13 16:22 GMT+02:00 David T. Lewis <[hidden email]>:
>
> > On Sun, Jul 13, 2014 at 09:55:41AM -0400, David T. Lewis wrote:
> > > On Sun, Jul 06, 2014 at 12:47:25PM -0400, David T. Lewis wrote:
> > > > On Sun, Jul 06, 2014 at 12:23:11PM -0400, David T. Lewis wrote:
> > > > > On Sun, Jul 06, 2014 at 10:34:15AM +0200, Nicolas Cellier wrote:
> > > > > > 2014-07-04 0:47 GMT+02:00 Nicolas Cellier <
> > > > > > [hidden email]>:
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > 2014-07-04 0:09 GMT+02:00 Eliot Miranda <[hidden email]
> > >:
> > > > > > >
> > > > > > >
> > > > > > >>
> > > > > > >>
> > > > > > >> On Thu, Jul 3, 2014 at 12:33 PM, Nicolas Cellier <
> > > > > > >> [hidden email]> wrote:
> > > > > > >>
> > > > > > >>> The bug is repeatable, i simply have to execute this snippet
> > with my
> > > > > > >>> test file:
> > > > > > >>>
> > > > > > >>> (FileStream fileNamed: 'snapshot.bin') binary
> > contentsOfEntireFile
> > > > > > >>> asString zipped.
> > > > > > >>>
> > > > > > >>> The file is too big for an attachment here - 7.3 Mbytes - or
> > 1.7 Mbytes
> > > > > > >>> gzipped by external tool.
> > > > > > >>> If someone can suggest an upload site, or want it by mail,
> > just ask.
> > > > > > >>>
> > > > > > >>
> > > > > > >> You're welcome to put it on ftp.mirandanamda.org, cogftpuser,
> > pw cogging
> > > > > > >> with 0's & 1's.
> > > > > > >>
> > > > > > >>
> > > > > > > done, pollution of your site engaged...
> > > > > > > Thanks Eliot!
> > > > > > >
> > > > > > >
> > > > > > >>
> > > > > > I inquired a bit more about this bug.
> > > > > > An important clue is that it does not happen in Pharo3.0!
> > > > > >
> > > > > > But Pharo3.0 did not fundamentally change compression
> > > > > > (except some FileSystem related changes, separation of CRC stuff
> > in another
> > > > > > package, some other cosmetic changes like replacing some self
> > assert: ...
> > > > > > by [...] assert and a potential bug in
> > > > > > DeflateStream>>nextPutAll:startingAt: introduced by CamilleTeruel).
> > > > > > Squeak behavior is presumably not due to a Compression change.
> > > > > >
> > > > > > Neither is it a VM problem, the bugs still shows up when running
> > the Squeak
> > > > > > image with Pharo VM.
> > > > > >
> > > > >
> > > > > Yes, it is definite a problem in the Squeak trunk image.
> > > > >
> > > > >
> > > > > > So the difference lies somewhere else: in our beloved Stream.
> > > > > > I remembered a recent change of Eliot related to handling of
> > Stream created
> > > > > > with on:from:to: (Collections-eem.567).
> > > > > > Reverting those changes just make the snippet pass!
> > > > > >
> > > > > > Ah ah! At the time, i found the change of Eliot quite minimal and
> > nice.
> > > > > > But I wish I had time to analyze this small innocent change deeper.
> > > > > > Indeed, I remembered I previously broke my teeth on this one 2
> > years or so
> > > > > > ago, and preferred to adopt another strategy: avoid using
> > on:from:to: and
> > > > > > ReadWriteStream as much as possible.
> > > > > > Why? because when analyzing the usage of inst.var. in Stream
> > hierarchy, it
> > > > > > gave me headaches, some subclass are just ignoring superclass
> > variables, or
> > > > > > worse are bending the semantics of superclass variables.
> > > > > > I came to the conclusion that I could not decently master a change
> > of
> > > > > > on:from:to:
> > > > >
> > > > > I can not confirm this. I have an image in which I can reliably
> > reproduce the
> > > > > failure in writing the MCZ to a file. I tried reverting the methods
> > that were
> > > > > introduced in Collections-eem.567 and I am still getting the same
> > failure.
> > > > >
> > > > > Dave
> > > >
> > > > Oops, I spoke too soon. Indeed the problem appears as of
> > Collections-eem.567,
> > > > and does not appear in Collections-nice.566.
> > > >
> > > > So I am confirming Nicolas' observations ... now to find the bug :)
> > > >
> > > > Dave
> > > >
> > >
> > > This seems to be a rather tricky bug, and I don't think any of us knows
> > the
> > > cause right now. In case anyone else wants to have a look at it, here is
> > a
> > > recipe for reproducing the bug in a clean 4.5 image:
> > >
> > > - Start with a clean Squeak 4.5 image in and empty working directory.
> > >       ftp://ftp.squeak.org/4.5/Squeak4.5-13680.zip
> > >
> > > - Open the image, do not update it.
> > >
> > > - Open an MC browser and add the http://source.squeak.org/trunk
> > repository for the Help-Squeak-Project package and Collections package.
> > >
> > > - Load Help-Squeak-Project-kfr.18 from trunk.
> > >
> > > - Load Collections-eem.567 from trunk.
> > >
> > > - Open a browser on class SqueakToolsDebuggerHelp, and delete the class
> > side method #showDebuggerMenuForm.
> > >
> > > - Remove the http://source.squeak.org/trunk repository form the
> > Help-Squeak-Project package in the MC browser.
> > >
> > > - Highlight your package-cache repository and try to save
> > Help-Squeak-project to the package-cache repository.
> > >
> > > - Enter author initials 'dtl'.
> > >
> > > - For the package comment, enter 'Remove unreferenced method'.
> > >
> > > - Package save will fail part way through saving the MCZ.
> > >
> > > - To reproduce the failure in this image, do the following in a
> > workspace:
> > >
> > >       mcv := MCVersion allInstances detect: [:e | e name = 'a
> > MCVersion(Help-Squeak-Project-dtl.19)'].
> > >       [f := FileStream fileNamed: 'junk.mcz'.
> > >       mcv fileOutOn: f]
> > >               ensure: [f close].
> > >
> >
> > In addition to the above recipe, here are some other observations:
> >
> > - Reverting from Collections-eem.567 to Collections-nice.566 makes the
> >   problem go away, but ONLY if the DeflatePlugin is active.
> >
> > - The failure appears while executing the fallback code for
> > #primitiveDeflateBlock
> >   in ZipWriteStream>>deflateBlock:chainLength:goodMatch:
> >
> > - If this primitive is deactivate by commenting out the
> >   <primitive: 'primitiveDeflateBlock' module: 'ZipPlugin'> in
> >   ZipWriteStream>>deflateBlock:chainLength:goodMatch: then the problem will
> >   occur regardless of which version of Collections is loaded.
> >
> > - When Collections-eem.567 is loaded, the primitive will fail on the 22nd
> >   call to #deflateBlock:chainLength:goodMatch: and a debugger appears
> > during
> >   execution of the fallback code.
> >
> > - When Collections-nice.566 is loaded, the primitive does not fail on the
> > 22nd
> >   call to #deflateBlock:chainLength:goodMatch:, the fallback code is never
> > run,
> >   and no error appears.
> >
> > - When Collections-nice.566 is loaded and the primitive is disabled to
> > force
> >   use of the fallback code, the error appears on on the 22nd call to
> >   #deflateBlock:chainLength:goodMatch: in exactlly the same place as when
> >   Collections-eem.567 is loaded.
> >
> > - If I inspect the ZipWriteStream at the failure point (when the debugger
> > pops
> >   up after the problem), I see no obvious difference in the state of the
> > stream
> >   between the Collections-eem.567 case versus the Collections-nice.566 with
> >   primitive disabled case.
> >
> > Dave
> >
> >
> Very good mining work!
> When I hear  Stream, I think fluid...
> But these snags make it so viscous, it's like our Stream is going to freeze
> soon.
> Or is it more than frozen? The vein you're after sounds as hard as diamond,
> we gonna need a sharp peak pickaxe.
>

I keep hoping that your stream fixes will somehow make this problem go away, but
maybe we are not so lucky.

Whenever I get more time to play with this, I think I will try to catch it with
gdb in the primitive. There is something strange happening that seems to first
be detectable in the primitive failure, but only if Eliot's image fix is present.

This kind of problem is a nice way to spend the morning with a good cup of coffee,
as some people do with the NY Times crossword puzzle ;)

   http://en.wikipedia.org/wiki/The_New_York_Times_crossword_puzzle

Another "morning coffee" project is to figure out which version of the LargeIntegersPlugin
should be used, which of course was the original topic that started this thread.
 
So given that the change adds an instance variable to WriteStream could that affect some plugin that somehow accesses a stream?  What plugin could that be?  I don't see any obvious inst vars in the DeflatePlugin or InflatePlugin.  To remind ourselves, the three changes are

add inst var to WriteStream:

PositionableStream subclass: #WriteStream
instanceVariableNames: 'writeLimit initialPositionOrNil'
classVariableNames: ''
poolDictionaries: ''
category: 'Collections-Streams'

use it in two places:
contents
"Answer with a copy of my collection from the start to the current position."

readLimit := readLimit max: position.
^collection copyFrom: (initialPositionOrNil ifNil: [1]) to: position

on: aCollection from: firstIndex to: lastIndex

| len |
collection := aCollection.
readLimit := 
writeLimit := lastIndex > (len := collection size)
ifTrue: [len]
ifFalse: [lastIndex].
position := firstIndex <= 1
ifTrue: [0]
ifFalse: [firstIndex - 1].
initialPositionOrNil := position + 1

What I've just found is that if I revert all three changes to WriteStream the bug goes away, but if I only revert the two method changes the bug remains.  So I think the problem is merely the adding of the inst var.  This could break some plugin which was expecting inst vars in subclasses of WriteStream to be at particular offsets determined by WriteStream instSize = 4 now being 5.  The question is which plugin(s)?

Doh!
 
DeflatePlugin>>loadDeflateStreamFrom: rcvr
| oop |
<inline: false>
((interpreterProxy isPointers: rcvr)
and: [(interpreterProxy slotSizeOf: rcvr) >= 15]) ifFalse:
[^false].
oop := interpreterProxy fetchPointer: 0 ofObject: rcvr.
(interpreterProxy isBytes: oop) ifFalse:
[^false].
zipCollection := interpreterProxy firstIndexableField: oop.
zipCollectionSize := interpreterProxy byteSizeOf: oop.

zipPosition := interpreterProxy fetchInteger: 1 ofObject: rcvr.
zipReadLimit := interpreterProxy fetchInteger: 2 ofObject: rcvr.
"zipWriteLimit := interpreterProxy fetchInteger: 3 ofObject: rcvr."

oop := interpreterProxy fetchPointer: 4 ofObject: rcvr.
((interpreterProxy isWords: oop)
and: [(interpreterProxy slotSizeOf: oop) = DeflateHashTableSize]) ifFalse:
[^false].
zipHashHead := interpreterProxy firstIndexableField: oop.
oop := interpreterProxy fetchPointer: 5 ofObject: rcvr.
((interpreterProxy isWords: oop)
and: [(interpreterProxy slotSizeOf: oop) = DeflateWindowSize]) ifFalse:
[^false].
zipHashTail := interpreterProxy firstIndexableField: oop.
zipHashValue := interpreterProxy fetchInteger: 6 ofObject: rcvr.
zipBlockPos := interpreterProxy fetchInteger: 7 ofObject: rcvr.
"zipBlockStart := interpreterProxy fetchInteger: 8 ofObject: rcvr."
oop := interpreterProxy fetchPointer: 9 ofObject: rcvr.
(interpreterProxy isBytes: oop) ifFalse:
[^false].
zipLiteralSize := interpreterProxy slotSizeOf: oop.
zipLiterals := interpreterProxy firstIndexableField: oop.

oop := interpreterProxy fetchPointer: 10 ofObject: rcvr.
((interpreterProxy isWords: oop)
and: [(interpreterProxy slotSizeOf: oop) >= zipLiteralSize]) ifFalse:
[^false].
zipDistances := interpreterProxy firstIndexableField: oop.

oop := interpreterProxy fetchPointer: 11 ofObject: rcvr.
((interpreterProxy isWords: oop)
and: [(interpreterProxy slotSizeOf: oop) = DeflateMaxLiteralCodes]) ifFalse:
[^false].
zipLiteralFreq := interpreterProxy firstIndexableField: oop.

oop := interpreterProxy fetchPointer: 12 ofObject: rcvr.
((interpreterProxy isWords: oop)
and: [(interpreterProxy slotSizeOf: oop) = DeflateMaxDistanceCodes]) ifFalse:
[^false].
zipDistanceFreq := interpreterProxy firstIndexableField: oop.

zipLiteralCount := interpreterProxy fetchInteger: 13 ofObject: rcvr.
zipMatchCount := interpreterProxy fetchInteger: 14 ofObject: rcvr.

^interpreterProxy failed not

I propose to add a variable that holds the inst size of the superclass of InflateStream, and to use this to correct the offsets.  Objections?
--
best,
Eliot



--
best,
Eliot

ZipPluginFixes.st (18K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] Re: [Vm-dev] Bug in writing compressed stream when saving an mcz (was: New Cog VMs available...)

David T. Lewis
In reply to this post by Eliot Miranda-2
 
On Fri, Jul 18, 2014 at 02:06:18PM -0700, Eliot Miranda wrote:
>  
> I think we have to accept that the fix to on:from:to: means that exsting
> VMs are broken.  Certainly the ZipPlugin (InflatePlugin & DeflatePlugin)'s
> dependency on WriteStream and ReadStream inst size is restrictive.  I'd
> rather lift the restriction and use new VMs than live with the restriction
> long-term.

Is this fix is of sufficient urgency to justify asking all Squeak trunk users
to replace their VMs? Is there any less disruptive way to address the issue?
I don't know the answers, just asking.

Dave

Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] Re: [Vm-dev] Bug in writing compressed stream when saving an mcz (was: New Cog VMs available...)

Eliot Miranda-2
 



On Fri, Jul 18, 2014 at 4:20 PM, David T. Lewis <[hidden email]> wrote:

On Fri, Jul 18, 2014 at 02:06:18PM -0700, Eliot Miranda wrote:
>
> I think we have to accept that the fix to on:from:to: means that exsting
> VMs are broken.  Certainly the ZipPlugin (InflatePlugin & DeflatePlugin)'s
> dependency on WriteStream and ReadStream inst size is restrictive.  I'd
> rather lift the restriction and use new VMs than live with the restriction
> long-term.

Is this fix is of sufficient urgency to justify asking all Squeak trunk users
to replace their VMs? Is there any less disruptive way to address the issue?
I don't know the answers, just asking.

Well, I've fixed the VM (commit to appear shortly) and much prefer having a fixed plugin.  The alternatives are
a) having some horrible hack in the image to store initialPositionOrNil out of streams
b) living with on:from:to: being broken

Since VMs can be fixed it seems to me that the best solution is to build new VMs.  That's the solution I'm following anyway.
--
best,
Eliot
Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] Re: [Vm-dev] Bug in writing compressed stream when saving an mcz (was: New Cog VMs available...)

Eliot Miranda-2
In reply to this post by Eliot Miranda-2
 
Hi All,

   find the fix attached.  I hope we can fold this into the standard VM soon.


On Fri, Jul 18, 2014 at 2:39 PM, Eliot Miranda <[hidden email]> wrote:
Hi All,

    I'm close to a fix on this, but I have to stop as I've some errands to run.  I wonder if anyone is interested in reviewing the code.  There's a mistake in there somewhere.  Careful review will help.

The issue is to match the inst var offsets in the InflatePlugin and DeflatePlugin methods with those in InflateStream DeflateStream ZipWriteStream and ZipEncoder now that the plugin code uses readStreamInstSize and writeStreamInstSize to get at them.  Perhaps some kind soul can read this code like a lawyer and spot my mistake?  I'll try and take another look at this this evening.

Changes attached and included for your perusal:

'From Squeak4.5 of 18 July 2014 [latest update: #13848] on 18 July 2014 at 2:34:04 pm'!
InflatePlugin subclass: #DeflatePlugin
instanceVariableNames: 'zipHashHead zipHashTail zipHashValue zipBlockPos zipBlockStart zipLiterals zipDistances zipLiteralFreq zipDistanceFreq zipLiteralCount zipLiteralSize zipMatchCount zipMatchLengthCodes zipDistanceCodes zipCrcTable zipExtraLengthBits zipExtraDistanceBits zipBaseLength zipBaseDistance writeStreamInstSize'
classVariableNames: 'DeflateHashBits DeflateHashMask DeflateHashShift DeflateHashTableSize DeflateMaxDistance DeflateMaxDistanceCodes DeflateMaxLiteralCodes DeflateMaxMatch DeflateMinMatch DeflateWindowMask DeflateWindowSize'
poolDictionaries: ''
category: 'VMMaker-Plugins'!
!DeflatePlugin commentStamp: 'tpr 5/5/2003 11:52' prior: 0!
This adds Zip deflating support.
InflatePlugin should not be translated but this subclass should since it is incorporated within that class's translation process!

!DeflatePlugin methodsFor: 'primitive support' stamp: 'eem 7/18/2014 13:56'!
determineSizeOfWriteStream: rcvr
"Determine the inst size of the class above DeflateStream or
ZipEncoder by looking for the first class whose inst size is less than 7."
| class |
class := interpreterProxy fetchClassOf: rcvr.
[class ~= interpreterProxy nilObject
and: [(interpreterProxy instanceSizeOf: class) >= 7]] whileTrue:
[class := interpreterProxy superclassOf: class].
class = interpreterProxy nilObject ifTrue:
[^false].
writeStreamInstSize := interpreterProxy instanceSizeOf: class.
^true
! !

!DeflatePlugin methodsFor: 'primitive support' stamp: 'eem 7/18/2014 14:15'!
loadDeflateStreamFrom: rcvr
| oop |
<inline: false>
((interpreterProxy isPointers: rcvr)
and: [(interpreterProxy slotSizeOf: rcvr) >= 15]) ifFalse:
[^false].
oop := interpreterProxy fetchPointer: 0 ofObject: rcvr.
(interpreterProxy isBytes: oop) ifFalse:
[^false].
writeStreamInstSize = 0 ifTrue:
[(self determineSizeOfWriteStream: rcvr) ifFalse:
[^false].
"If the receiver wasn't valid then we derived writeStreamInstSize from an invalid source.  discard it."
(interpreterProxy slotSizeOf: rcvr) < (writeStreamInstSize + 5) ifTrue:
[writeStreamInstSize := 0.
^false]].
zipCollection := interpreterProxy firstIndexableField: oop.
zipCollectionSize := interpreterProxy byteSizeOf: oop.

zipPosition := interpreterProxy fetchInteger: 1 ofObject: rcvr.
zipReadLimit := interpreterProxy fetchInteger: 2 ofObject: rcvr.
"zipWriteLimit := interpreterProxy fetchInteger: 3 ofObject: rcvr."

oop := interpreterProxy fetchPointer: writeStreamInstSize + 0 ofObject: rcvr.
((interpreterProxy isWords: oop)
and: [(interpreterProxy slotSizeOf: oop) = DeflateHashTableSize]) ifFalse:
[^false].
zipHashHead := interpreterProxy firstIndexableField: oop.
oop := interpreterProxy fetchPointer: writeStreamInstSize + 1 ofObject: rcvr.
((interpreterProxy isWords: oop)
and: [(interpreterProxy slotSizeOf: oop) = DeflateWindowSize]) ifFalse:
[^false].
zipHashTail := interpreterProxy firstIndexableField: oop.
zipHashValue := interpreterProxy fetchInteger: writeStreamInstSize + 2 ofObject: rcvr.
zipBlockPos := interpreterProxy fetchInteger: writeStreamInstSize + 3 ofObject: rcvr.
"zipBlockStart := interpreterProxy fetchInteger: writeStreamInstSize + 4 ofObject: rcvr."
oop := interpreterProxy fetchPointer: writeStreamInstSize + 5 ofObject: rcvr.
(interpreterProxy isBytes: oop) ifFalse:
[^false].
zipLiteralSize := interpreterProxy slotSizeOf: oop.
zipLiterals := interpreterProxy firstIndexableField: oop.

oop := interpreterProxy fetchPointer: writeStreamInstSize + 6 ofObject: rcvr.
((interpreterProxy isWords: oop)
and: [(interpreterProxy slotSizeOf: oop) >= zipLiteralSize]) ifFalse:
[^false].
zipDistances := interpreterProxy firstIndexableField: oop.

oop := interpreterProxy fetchPointer: writeStreamInstSize + 7 ofObject: rcvr.
((interpreterProxy isWords: oop)
and: [(interpreterProxy slotSizeOf: oop) = DeflateMaxLiteralCodes]) ifFalse:
[^false].
zipLiteralFreq := interpreterProxy firstIndexableField: oop.

oop := interpreterProxy fetchPointer: writeStreamInstSize + 8 ofObject: rcvr.
((interpreterProxy isWords: oop)
and: [(interpreterProxy slotSizeOf: oop) = DeflateMaxDistanceCodes]) ifFalse:
[^false].
zipDistanceFreq := interpreterProxy firstIndexableField: oop.

zipLiteralCount := interpreterProxy fetchInteger: writeStreamInstSize + 9 ofObject: rcvr.
zipMatchCount := interpreterProxy fetchInteger: writeStreamInstSize + 10 ofObject: rcvr.

^interpreterProxy failed not! !

!DeflatePlugin methodsFor: 'primitive support' stamp: 'eem 7/18/2014 14:15'!
loadZipEncoderFrom: rcvr
| oop |
<inline: false>
writeStreamInstSize = 0 ifTrue:
[(self determineSizeOfWriteStream: rcvr) ifFalse:
[^false].
"If the receiver wasn't valid then we derived writeStreamInstSize from an invalid source.  discard it."
(interpreterProxy slotSizeOf: rcvr) < (writeStreamInstSize + 3) ifTrue:
[writeStreamInstSize := 0.
^false]].
((interpreterProxy isPointers: rcvr)
and: [(interpreterProxy slotSizeOf: rcvr) >= (writeStreamInstSize + 3)]) ifFalse:
[^false].
oop := interpreterProxy fetchPointer: 0 ofObject: rcvr.
(interpreterProxy isBytes: oop) ifFalse:
[^interpreterProxy primitiveFail].
zipCollection := interpreterProxy firstIndexableField: oop.
zipCollectionSize := interpreterProxy byteSizeOf: oop.

zipPosition := interpreterProxy fetchInteger: 1 ofObject: rcvr.
zipReadLimit := interpreterProxy fetchInteger: 2 ofObject: rcvr.
"zipWriteLimit := interpreterProxy fetchInteger: 3 ofObject: rcvr."
zipBitBuf := interpreterProxy fetchInteger: writeStreamInstSize + 0 ofObject: rcvr.
zipBitPos := interpreterProxy fetchInteger: writeStreamInstSize + 1 ofObject: rcvr.

^interpreterProxy failed not! !

!DeflatePlugin methodsFor: 'primitives' stamp: 'eem 7/18/2014 14:28'!
primitiveDeflateBlock
"Primitive. Deflate the current contents of the receiver."
| goodMatch chainLength lastIndex rcvr result |
<export: true>
<inline: false>
interpreterProxy methodArgumentCount = 3
ifFalse:[^interpreterProxy primitiveFail].
goodMatch := interpreterProxy stackIntegerValue: 0.
chainLength := interpreterProxy stackIntegerValue: 1.
lastIndex := interpreterProxy stackIntegerValue: 2.
rcvr := interpreterProxy stackObjectValue: 3.
interpreterProxy failed ifTrue:[^nil].
self cCode:'' inSmalltalk:[
zipMatchLengthCodes := CArrayAccessor on: ZipWriteStream matchLengthCodes.
zipDistanceCodes := CArrayAccessor on: ZipWriteStream distanceCodes].
(self loadDeflateStreamFrom: rcvr)
ifFalse:[^interpreterProxy primitiveFail].
result := self deflateBlock: lastIndex chainLength: chainLength goodMatch: goodMatch.
interpreterProxy failed ifFalse:[
"Store back modified values"
interpreterProxy storeInteger: writeStreamInstSize + 2 ofObject: rcvr withValue: zipHashValue.
interpreterProxy storeInteger: writeStreamInstSize + 3 ofObject: rcvr withValue: zipBlockPos.
interpreterProxy storeInteger: writeStreamInstSize + 9 ofObject: rcvr withValue: zipLiteralCount.
interpreterProxy storeInteger: writeStreamInstSize + 10 ofObject: rcvr withValue: zipMatchCount].
interpreterProxy failed ifFalse:[
interpreterProxy pop: 4.
interpreterProxy pushBool: result.
].! !

!DeflatePlugin methodsFor: 'primitives' stamp: 'eem 7/18/2014 14:24'!
primitiveZipSendBlock
| distTree litTree distStream litStream rcvr result |
<export: true>
interpreterProxy methodArgumentCount = 4 
ifFalse:[^interpreterProxy primitiveFail].
distTree := interpreterProxy stackObjectValue: 0.
litTree := interpreterProxy stackObjectValue: 1.
distStream := interpreterProxy stackObjectValue: 2.
litStream := interpreterProxy stackObjectValue: 3.
rcvr := interpreterProxy stackObjectValue: 4.
interpreterProxy failed ifTrue:[^nil].
(self loadZipEncoderFrom: rcvr)
ifFalse:[^interpreterProxy primitiveFail].
((interpreterProxy isPointers: distTree) and:[
(interpreterProxy slotSizeOf: distTree) >= 2])
ifFalse:[^interpreterProxy primitiveFail].
((interpreterProxy isPointers: litTree) and:[
(interpreterProxy slotSizeOf: litTree) >= 2])
ifFalse:[^interpreterProxy primitiveFail].
((interpreterProxy isPointers: litStream) and:[
(interpreterProxy slotSizeOf: litStream) >= 3])
ifFalse:[^interpreterProxy primitiveFail].
((interpreterProxy isPointers: distStream) and:[
(interpreterProxy slotSizeOf: distStream) >= 3])
ifFalse:[^interpreterProxy primitiveFail].
self cCode:'' inSmalltalk:[
zipMatchLengthCodes := CArrayAccessor on: ZipWriteStream matchLengthCodes.
zipDistanceCodes := CArrayAccessor on: ZipWriteStream distanceCodes.
zipExtraLengthBits := CArrayAccessor on: ZipWriteStream extraLengthBits.
zipExtraDistanceBits := CArrayAccessor on: ZipWriteStream extraDistanceBits.
zipBaseLength := CArrayAccessor on: ZipWriteStream baseLength.
zipBaseDistance := CArrayAccessor on: ZipWriteStream baseDistance].
result := self sendBlock: litStream with: distStream with: litTree with: distTree.
interpreterProxy failed ifFalse:[
interpreterProxy storeInteger: 1 ofObject: rcvr withValue: zipPosition.
interpreterProxy storeInteger: readStreamInstSize + 1 ofObject: rcvr withValue: zipBitBuf.
interpreterProxy storeInteger: readStreamInstSize + 2 ofObject: rcvr withValue: zipBitPos.
].
interpreterProxy failed ifFalse:[
interpreterProxy pop: 5. "rcvr + args"
interpreterProxy pushInteger: result.
].! !
InterpreterPlugin subclass: #InflatePlugin
instanceVariableNames: 'zipCollection zipReadLimit zipPosition zipState zipBitBuf zipBitPos zipSource zipSourcePos zipSourceLimit zipLitTable zipDistTable zipCollectionSize zipLitTableSize zipDistTableSize readStreamInstSize'
classVariableNames: 'MaxBits StateNoMoreData'
poolDictionaries: ''
category: 'VMMaker-Plugins'!
!InflatePlugin commentStamp: '<historical>' prior: 0!
This plugin implements the one crucial function for efficiently decompressing streams.!

!InflatePlugin methodsFor: 'primitive support' stamp: 'eem 7/18/2014 14:02'!
determineSizeOfReadStream: rcvr
"Determine the inst size of the class above DeflateStream by
looking for the first class whose inst size is less than 14."
| class |
class := interpreterProxy fetchClassOf: rcvr.
[class ~= interpreterProxy nilObject
and: [(interpreterProxy instanceSizeOf: class) >= 14]] whileTrue:
[class := interpreterProxy superclassOf: class].
class = interpreterProxy nilObject ifTrue:
[^false].
readStreamInstSize := interpreterProxy instanceSizeOf: class.
^true! !

!InflatePlugin methodsFor: 'primitives' stamp: 'eem 7/18/2014 14:18'!
primitiveInflateDecompressBlock
"Primitive. Inflate a single block."
| oop rcvr |
<export: true>
interpreterProxy methodArgumentCount = 2 ifFalse:
[^interpreterProxy primitiveFail].
"distance table"
oop := interpreterProxy stackValue: 0.
(interpreterProxy isWords: oop) ifFalse:
[^interpreterProxy primitiveFail].
zipDistTable := interpreterProxy firstIndexableField: oop.
zipDistTableSize := interpreterProxy slotSizeOf: oop.

"literal table"
oop := interpreterProxy stackValue: 1.
(interpreterProxy isWords: oop) ifFalse:
[^interpreterProxy primitiveFail].
zipLitTable := interpreterProxy firstIndexableField: oop.
zipLitTableSize := interpreterProxy slotSizeOf: oop.


"Receiver (InflateStream)"
rcvr := interpreterProxy stackValue: 2.
(interpreterProxy isPointers: rcvr) ifFalse:
[^interpreterProxy primitiveFail].
"All the integer instvars"
readStreamInstSize = 0 ifTrue:
[(self determineSizeOfReadStream: rcvr) ifFalse:
[^interpreterProxy primitiveFail].
"If the receiver wasn't valid then we derived readStreamInstSize from an invalid source.  discard it."
(interpreterProxy slotSizeOf: rcvr) < (readStreamInstSize + 6) ifTrue:
[readStreamInstSize := 0.
^interpreterProxy primitiveFail]].
(interpreterProxy slotSizeOf: rcvr) < (readStreamInstSize + 7) ifTrue:
[^interpreterProxy primitiveFail].

zipReadLimit := interpreterProxy fetchInteger: 2 ofObject: rcvr.
zipState := interpreterProxy fetchInteger: readStreamInstSize + 0 ofObject: rcvr.
zipBitBuf := interpreterProxy fetchInteger: readStreamInstSize + 1 ofObject: rcvr.
zipBitPos := interpreterProxy fetchInteger: readStreamInstSize + 2 ofObject: rcvr.
zipSourcePos := interpreterProxy fetchInteger: readStreamInstSize + 4 ofObject: rcvr.
zipSourceLimit := interpreterProxy fetchInteger: readStreamInstSize + 5 ofObject: rcvr.
interpreterProxy failed ifTrue:[^nil].
zipReadLimit := zipReadLimit - 1.
zipSourcePos := zipSourcePos - 1.
zipSourceLimit := zipSourceLimit - 1.

"collection"
oop := interpreterProxy fetchPointer: 0 ofObject: rcvr.
(interpreterProxy isBytes: oop) ifFalse:
[^interpreterProxy primitiveFail].
zipCollection := interpreterProxy firstIndexableField: oop.
zipCollectionSize := interpreterProxy byteSizeOf: oop.

"source"
oop := interpreterProxy fetchPointer: readStreamInstSize + 3 ofObject: rcvr.
(interpreterProxy isBytes: oop) ifFalse:
[^interpreterProxy primitiveFail].
zipSource := interpreterProxy firstIndexableField: oop.

"do the primitive"
self zipDecompressBlock.
interpreterProxy failed ifFalse: "store modified values back"
[interpreterProxy storeInteger: 2 ofObject: rcvr withValue: zipReadLimit + 1.
interpreterProxy storeInteger: readStreamInstSize + 0 ofObject: rcvr withValue: zipState.
interpreterProxy storeInteger: readStreamInstSize + 1 ofObject: rcvr withValue: zipBitBuf.
interpreterProxy storeInteger: readStreamInstSize + 2 ofObject: rcvr withValue: zipBitPos.
interpreterProxy storeInteger: readStreamInstSize + 4 ofObject: rcvr withValue: zipSourcePos + 1.
interpreterProxy pop: 2]! !



On Fri, Jul 18, 2014 at 12:46 PM, Eliot Miranda <[hidden email]> wrote:



On Fri, Jul 18, 2014 at 12:38 PM, Eliot Miranda <[hidden email]> wrote:
Stupid me.  It's got to be the DeflateStream, and its subclasses ZipWriteStream, GZipWriteStream and ZLibWriteStream.  So my change screws all uses of these in plugins.  So what to do about fixing this?  I would like to have a go at fixing the plugins.  But cheaper might be to fix WriteStream with some property hack, storing the initialPosition somewhere else (a class side weak dictionary of stream -> initialPosition ?? (yuck)).


On Fri, Jul 18, 2014 at 12:31 PM, Eliot Miranda <[hidden email]> wrote:



On Thu, Jul 17, 2014 at 4:02 PM, David T. Lewis <[hidden email]> wrote:
On Fri, Jul 18, 2014 at 12:24:29AM +0200, Nicolas Cellier wrote:
> 2014-07-13 16:22 GMT+02:00 David T. Lewis <[hidden email]>:
>
> > On Sun, Jul 13, 2014 at 09:55:41AM -0400, David T. Lewis wrote:
> > > On Sun, Jul 06, 2014 at 12:47:25PM -0400, David T. Lewis wrote:
> > > > On Sun, Jul 06, 2014 at 12:23:11PM -0400, David T. Lewis wrote:
> > > > > On Sun, Jul 06, 2014 at 10:34:15AM +0200, Nicolas Cellier wrote:
> > > > > > 2014-07-04 0:47 GMT+02:00 Nicolas Cellier <
> > > > > > [hidden email]>:
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > 2014-07-04 0:09 GMT+02:00 Eliot Miranda <[hidden email]
> > >:
> > > > > > >
> > > > > > >
> > > > > > >>
> > > > > > >>
> > > > > > >> On Thu, Jul 3, 2014 at 12:33 PM, Nicolas Cellier <
> > > > > > >> [hidden email]> wrote:
> > > > > > >>
> > > > > > >>> The bug is repeatable, i simply have to execute this snippet
> > with my
> > > > > > >>> test file:
> > > > > > >>>
> > > > > > >>> (FileStream fileNamed: 'snapshot.bin') binary
> > contentsOfEntireFile
> > > > > > >>> asString zipped.
> > > > > > >>>
> > > > > > >>> The file is too big for an attachment here - 7.3 Mbytes - or
> > 1.7 Mbytes
> > > > > > >>> gzipped by external tool.
> > > > > > >>> If someone can suggest an upload site, or want it by mail,
> > just ask.
> > > > > > >>>
> > > > > > >>
> > > > > > >> You're welcome to put it on ftp.mirandanamda.org, cogftpuser,
> > pw cogging
> > > > > > >> with 0's & 1's.
> > > > > > >>
> > > > > > >>
> > > > > > > done, pollution of your site engaged...
> > > > > > > Thanks Eliot!
> > > > > > >
> > > > > > >
> > > > > > >>
> > > > > > I inquired a bit more about this bug.
> > > > > > An important clue is that it does not happen in Pharo3.0!
> > > > > >
> > > > > > But Pharo3.0 did not fundamentally change compression
> > > > > > (except some FileSystem related changes, separation of CRC stuff
> > in another
> > > > > > package, some other cosmetic changes like replacing some self
> > assert: ...
> > > > > > by [...] assert and a potential bug in
> > > > > > DeflateStream>>nextPutAll:startingAt: introduced by CamilleTeruel).
> > > > > > Squeak behavior is presumably not due to a Compression change.
> > > > > >
> > > > > > Neither is it a VM problem, the bugs still shows up when running
> > the Squeak
> > > > > > image with Pharo VM.
> > > > > >
> > > > >
> > > > > Yes, it is definite a problem in the Squeak trunk image.
> > > > >
> > > > >
> > > > > > So the difference lies somewhere else: in our beloved Stream.
> > > > > > I remembered a recent change of Eliot related to handling of
> > Stream created
> > > > > > with on:from:to: (Collections-eem.567).
> > > > > > Reverting those changes just make the snippet pass!
> > > > > >
> > > > > > Ah ah! At the time, i found the change of Eliot quite minimal and
> > nice.
> > > > > > But I wish I had time to analyze this small innocent change deeper.
> > > > > > Indeed, I remembered I previously broke my teeth on this one 2
> > years or so
> > > > > > ago, and preferred to adopt another strategy: avoid using
> > on:from:to: and
> > > > > > ReadWriteStream as much as possible.
> > > > > > Why? because when analyzing the usage of inst.var. in Stream
> > hierarchy, it
> > > > > > gave me headaches, some subclass are just ignoring superclass
> > variables, or
> > > > > > worse are bending the semantics of superclass variables.
> > > > > > I came to the conclusion that I could not decently master a change
> > of
> > > > > > on:from:to:
> > > > >
> > > > > I can not confirm this. I have an image in which I can reliably
> > reproduce the
> > > > > failure in writing the MCZ to a file. I tried reverting the methods
> > that were
> > > > > introduced in Collections-eem.567 and I am still getting the same
> > failure.
> > > > >
> > > > > Dave
> > > >
> > > > Oops, I spoke too soon. Indeed the problem appears as of
> > Collections-eem.567,
> > > > and does not appear in Collections-nice.566.
> > > >
> > > > So I am confirming Nicolas' observations ... now to find the bug :)
> > > >
> > > > Dave
> > > >
> > >
> > > This seems to be a rather tricky bug, and I don't think any of us knows
> > the
> > > cause right now. In case anyone else wants to have a look at it, here is
> > a
> > > recipe for reproducing the bug in a clean 4.5 image:
> > >
> > > - Start with a clean Squeak 4.5 image in and empty working directory.
> > >       ftp://ftp.squeak.org/4.5/Squeak4.5-13680.zip
> > >
> > > - Open the image, do not update it.
> > >
> > > - Open an MC browser and add the http://source.squeak.org/trunk
> > repository for the Help-Squeak-Project package and Collections package.
> > >
> > > - Load Help-Squeak-Project-kfr.18 from trunk.
> > >
> > > - Load Collections-eem.567 from trunk.
> > >
> > > - Open a browser on class SqueakToolsDebuggerHelp, and delete the class
> > side method #showDebuggerMenuForm.
> > >
> > > - Remove the http://source.squeak.org/trunk repository form the
> > Help-Squeak-Project package in the MC browser.
> > >
> > > - Highlight your package-cache repository and try to save
> > Help-Squeak-project to the package-cache repository.
> > >
> > > - Enter author initials 'dtl'.
> > >
> > > - For the package comment, enter 'Remove unreferenced method'.
> > >
> > > - Package save will fail part way through saving the MCZ.
> > >
> > > - To reproduce the failure in this image, do the following in a
> > workspace:
> > >
> > >       mcv := MCVersion allInstances detect: [:e | e name = 'a
> > MCVersion(Help-Squeak-Project-dtl.19)'].
> > >       [f := FileStream fileNamed: 'junk.mcz'.
> > >       mcv fileOutOn: f]
> > >               ensure: [f close].
> > >
> >
> > In addition to the above recipe, here are some other observations:
> >
> > - Reverting from Collections-eem.567 to Collections-nice.566 makes the
> >   problem go away, but ONLY if the DeflatePlugin is active.
> >
> > - The failure appears while executing the fallback code for
> > #primitiveDeflateBlock
> >   in ZipWriteStream>>deflateBlock:chainLength:goodMatch:
> >
> > - If this primitive is deactivate by commenting out the
> >   <primitive: 'primitiveDeflateBlock' module: 'ZipPlugin'> in
> >   ZipWriteStream>>deflateBlock:chainLength:goodMatch: then the problem will
> >   occur regardless of which version of Collections is loaded.
> >
> > - When Collections-eem.567 is loaded, the primitive will fail on the 22nd
> >   call to #deflateBlock:chainLength:goodMatch: and a debugger appears
> > during
> >   execution of the fallback code.
> >
> > - When Collections-nice.566 is loaded, the primitive does not fail on the
> > 22nd
> >   call to #deflateBlock:chainLength:goodMatch:, the fallback code is never
> > run,
> >   and no error appears.
> >
> > - When Collections-nice.566 is loaded and the primitive is disabled to
> > force
> >   use of the fallback code, the error appears on on the 22nd call to
> >   #deflateBlock:chainLength:goodMatch: in exactlly the same place as when
> >   Collections-eem.567 is loaded.
> >
> > - If I inspect the ZipWriteStream at the failure point (when the debugger
> > pops
> >   up after the problem), I see no obvious difference in the state of the
> > stream
> >   between the Collections-eem.567 case versus the Collections-nice.566 with
> >   primitive disabled case.
> >
> > Dave
> >
> >
> Very good mining work!
> When I hear  Stream, I think fluid...
> But these snags make it so viscous, it's like our Stream is going to freeze
> soon.
> Or is it more than frozen? The vein you're after sounds as hard as diamond,
> we gonna need a sharp peak pickaxe.
>

I keep hoping that your stream fixes will somehow make this problem go away, but
maybe we are not so lucky.

Whenever I get more time to play with this, I think I will try to catch it with
gdb in the primitive. There is something strange happening that seems to first
be detectable in the primitive failure, but only if Eliot's image fix is present.

This kind of problem is a nice way to spend the morning with a good cup of coffee,
as some people do with the NY Times crossword puzzle ;)

   http://en.wikipedia.org/wiki/The_New_York_Times_crossword_puzzle

Another "morning coffee" project is to figure out which version of the LargeIntegersPlugin
should be used, which of course was the original topic that started this thread.
 
So given that the change adds an instance variable to WriteStream could that affect some plugin that somehow accesses a stream?  What plugin could that be?  I don't see any obvious inst vars in the DeflatePlugin or InflatePlugin.  To remind ourselves, the three changes are

add inst var to WriteStream:

PositionableStream subclass: #WriteStream
instanceVariableNames: 'writeLimit initialPositionOrNil'
classVariableNames: ''
poolDictionaries: ''
category: 'Collections-Streams'

use it in two places:
contents
"Answer with a copy of my collection from the start to the current position."

readLimit := readLimit max: position.
^collection copyFrom: (initialPositionOrNil ifNil: [1]) to: position

on: aCollection from: firstIndex to: lastIndex

| len |
collection := aCollection.
readLimit := 
writeLimit := lastIndex > (len := collection size)
ifTrue: [len]
ifFalse: [lastIndex].
position := firstIndex <= 1
ifTrue: [0]
ifFalse: [firstIndex - 1].
initialPositionOrNil := position + 1

What I've just found is that if I revert all three changes to WriteStream the bug goes away, but if I only revert the two method changes the bug remains.  So I think the problem is merely the adding of the inst var.  This could break some plugin which was expecting inst vars in subclasses of WriteStream to be at particular offsets determined by WriteStream instSize = 4 now being 5.  The question is which plugin(s)?

Doh!
 
DeflatePlugin>>loadDeflateStreamFrom: rcvr
| oop |
<inline: false>
((interpreterProxy isPointers: rcvr)
and: [(interpreterProxy slotSizeOf: rcvr) >= 15]) ifFalse:
[^false].
oop := interpreterProxy fetchPointer: 0 ofObject: rcvr.
(interpreterProxy isBytes: oop) ifFalse:
[^false].
zipCollection := interpreterProxy firstIndexableField: oop.
zipCollectionSize := interpreterProxy byteSizeOf: oop.

zipPosition := interpreterProxy fetchInteger: 1 ofObject: rcvr.
zipReadLimit := interpreterProxy fetchInteger: 2 ofObject: rcvr.
"zipWriteLimit := interpreterProxy fetchInteger: 3 ofObject: rcvr."

oop := interpreterProxy fetchPointer: 4 ofObject: rcvr.
((interpreterProxy isWords: oop)
and: [(interpreterProxy slotSizeOf: oop) = DeflateHashTableSize]) ifFalse:
[^false].
zipHashHead := interpreterProxy firstIndexableField: oop.
oop := interpreterProxy fetchPointer: 5 ofObject: rcvr.
((interpreterProxy isWords: oop)
and: [(interpreterProxy slotSizeOf: oop) = DeflateWindowSize]) ifFalse:
[^false].
zipHashTail := interpreterProxy firstIndexableField: oop.
zipHashValue := interpreterProxy fetchInteger: 6 ofObject: rcvr.
zipBlockPos := interpreterProxy fetchInteger: 7 ofObject: rcvr.
"zipBlockStart := interpreterProxy fetchInteger: 8 ofObject: rcvr."
oop := interpreterProxy fetchPointer: 9 ofObject: rcvr.
(interpreterProxy isBytes: oop) ifFalse:
[^false].
zipLiteralSize := interpreterProxy slotSizeOf: oop.
zipLiterals := interpreterProxy firstIndexableField: oop.

oop := interpreterProxy fetchPointer: 10 ofObject: rcvr.
((interpreterProxy isWords: oop)
and: [(interpreterProxy slotSizeOf: oop) >= zipLiteralSize]) ifFalse:
[^false].
zipDistances := interpreterProxy firstIndexableField: oop.

oop := interpreterProxy fetchPointer: 11 ofObject: rcvr.
((interpreterProxy isWords: oop)
and: [(interpreterProxy slotSizeOf: oop) = DeflateMaxLiteralCodes]) ifFalse:
[^false].
zipLiteralFreq := interpreterProxy firstIndexableField: oop.

oop := interpreterProxy fetchPointer: 12 ofObject: rcvr.
((interpreterProxy isWords: oop)
and: [(interpreterProxy slotSizeOf: oop) = DeflateMaxDistanceCodes]) ifFalse:
[^false].
zipDistanceFreq := interpreterProxy firstIndexableField: oop.

zipLiteralCount := interpreterProxy fetchInteger: 13 ofObject: rcvr.
zipMatchCount := interpreterProxy fetchInteger: 14 ofObject: rcvr.

^interpreterProxy failed not

I propose to add a variable that holds the inst size of the superclass of InflateStream, and to use this to correct the offsets.  Objections?
--
best,
Eliot



--
best,
Eliot



--
best,
Eliot

ZipPluginFixes.st (20K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] Re: [Vm-dev] Bug in writing compressed stream when saving an mcz (was: New Cog VMs available...)

timrowledge
In reply to this post by Eliot Miranda-2

The *plugin* is fixed and can be provided as a fix for current vms. No need to require entire new vms. It's what the plugin idea was invented for... So long as we haven't broken the functionality since  '99 or whenever it was that Andreas & I added it.

Looks like we have several avenues for fixes;
an image update that removes the problematic change pro tem
A plugin solus patch that makes the image as-is Just Work
Maybe the plugin and some image code to detect it, use it if possible, alternative st code otherwise?
And of course, replacement vms.

Hopefully it is not beyond our collective wit to make an update that fetches the correct plugin and installs it.

/tim
{insert witticism here}
Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] Re: [Vm-dev] Bug in writing compressed stream when saving an mcz (was: New Cog VMs available...)

Eliot Miranda-2
 



On Fri, Jul 18, 2014 at 6:42 PM, tim Rowledge <[hidden email]> wrote:

The *plugin* is fixed and can be provided as a fix for current vms. No need to require entire new vms. It's what the plugin idea was invented for... So long as we haven't broken the functionality since  '99 or whenever it was that Andreas & I added it.

Looks like we have several avenues for fixes;
an image update that removes the problematic change pro tem
A plugin solus patch that makes the image as-is Just Work
Maybe the plugin and some image code to detect it, use it if possible, alternative st code otherwise?
And of course, replacement vms.

Hopefully it is not beyond our collective wit to make an update that fetches the correct plugin and installs it.

It is internal in the Cog VMs.  And before you condemn that choice it /is/ slightly more efficient as it doesn't have to indirect to access the interpreterProxy API.  But in any case new Cog VMs are already available.



/tim
{insert witticism here}



--
best,
Eliot
Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] Re: [Vm-dev] Bug in writing compressed stream when saving an mcz (was: New Cog VMs available...)

David T. Lewis
In reply to this post by Eliot Miranda-2
 
On Fri, Jul 18, 2014 at 05:26:52PM -0700, Eliot Miranda wrote:
>  
> Hi All,
>
>    find the fix attached.  I hope we can fold this into the standard VM
> soon.
>

Thanks Eliot, I'll update it this weekend in VMM trunk also.

Dave

Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] Re: [Vm-dev] Bug in writing compressed stream when saving an mcz (was: New Cog VMs available...)

timrowledge
In reply to this post by Eliot Miranda-2
 




It is internal in the Cog VMs.  And before you condemn that choice it /is/ slightly more efficient as it doesn't have to indirect to access the interpreterProxy API.  But in any case new Cog VMs are already available.


I know - your more direct macro access is a good idea. But that wasnt the point I intended; unless we've broken the plugin mechanism over the years a fixed-up zip plugin will over-ride the broken internal code, offering a very easy fix. It should even work in a running system where you don't want to quit and restart with a new vm for whatever reason. Something like 
Smalltalk unloadModule: 'ziplthingy'
Should disconnect from the old version and the next call to anything in the plugin ought to load the new external copy. Obviously this could be complicated by any stored state, so caveat emptor.

It's a really neat trick when developing a plugin.
Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] Re: [Vm-dev] Bug in writing compressed stream when saving an mcz (was: New Cog VMs available...)

Levente Uzonyi-2
In reply to this post by David T. Lewis
 
This is a good workaround for the problem, but I think the main issue is
that the VM has assumptions about the layout of an object.
I would simply pass all the parameters to the primitive. There's only 14
of them, so this could work. But this plugin also writes back the state to
the object, so that would create another problem.

I think the best would be to create new variants of #primitiveZipSendBlock
and #primitiveDeflateBlock which accept an array, and use that to
load and store the paramters of the compression algorithm. This way
objects with different layout would be able to use the plugin, and we
could get rid of these compression streams in the long term (or change
their layout without breaking them).

Another solution would be to use an external library(libraries) for
compression, but that's a lot more work.


Levente

P.S.: instead of finding the number of slots of WriteStream, it would have
been possible to index the slots from the end of the passed object. E.g.

loadDeflateStreamFrom: rcvr
  | slotSize oop |
  <inline: false>
  (interpreterProxy isPointers: rcvr) ifFalse: [ ^false ].
  slotSize := interpreterProxy slotSizeOf: rcvr.
  slotSize < 15 ifTrue: [ ^false ].
  ...
  zipCollection := interpreterProxy firstIndexableField: oop.
  zipCollectionSize := interpreterProxy byteSizeOf: oop.

  zipPosition := interpreterProxy fetchInteger: 1 ofObject: rcvr.
  zipReadLimit := interpreterProxy fetchInteger: 2 ofObject: rcvr.
  ...
  oop := interpreterProxy fetchPointer: slotSize - 14 ofObject: rcvr.
  ((interpreterProxy isWords: oop)
  and: [(interpreterProxy slotSizeOf: oop) = DeflateHashTableSize]) ifFalse:
  [^false].
  zipHashHead := interpreterProxy firstIndexableField: oop.
  oop := interpreterProxy fetchPointer: slotSize - 13 ofObject: rcvr.
  ((interpreterProxy isWords: oop)
  and: [(interpreterProxy slotSizeOf: oop) = DeflateWindowSize]) ifFalse:
  [^false].
  zipHashTail := interpreterProxy firstIndexableField: oop.
  zipHashValue := interpreterProxy fetchInteger: slotSize - 12 ofObject: rcvr.
  zipBlockPos := interpreterProxy fetchInteger: slotSize - 11 ofObject: rcvr.
  ...

Of course this would have its own cons.

On Sat, 19 Jul 2014, David T. Lewis wrote:

>
> On Fri, Jul 18, 2014 at 05:26:52PM -0700, Eliot Miranda wrote:
>>
>> Hi All,
>>
>>    find the fix attached.  I hope we can fold this into the standard VM
>> soon.
>>
>
> Thanks Eliot, I'll update it this weekend in VMM trunk also.
>
> Dave
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] Re: [Vm-dev] Bug in writing compressed stream when saving an mcz (was: New Cog VMs available...)

David T. Lewis
In reply to this post by timrowledge
 
On Sat, Jul 19, 2014 at 09:50:42AM -0700, tim Rowledge wrote:

>  
>
>
> >
> > It is internal in the Cog VMs.  And before you condemn that choice it /is/ slightly more efficient as it doesn't have to indirect to access the interpreterProxy API.  But in any case new Cog VMs are already available.
> >
> >
> I know - your more direct macro access is a good idea. But that wasnt the point I intended; unless we've broken the plugin mechanism over the years a fixed-up zip plugin will over-ride the broken internal code, offering a very easy fix. It should even work in a running system where you don't want to quit and restart with a new vm for whatever reason. Something like
> Smalltalk unloadModule: 'ziplthingy'
> Should disconnect from the old version and the next call to anything in the plugin ought to load the new external copy. Obviously this could be complicated by any stored state, so caveat emptor.
>
> It's a really neat trick when developing a plugin.

Yes, this mechanism does work, and it is quite feasible to distribute an
external plugin to override an existing internal plugin. You are right
that is it not necessary to distribute a new VM. From the point of view of
a Squeak user who may not be familiar with how the VM is put together, it
may be something of an academic point since we have no extablished process
for distributing plugin updates independent of the full VM installations.

Dave

Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] Re: [Vm-dev] Bug in writing compressed stream when saving an mcz (was: New Cog VMs available...)

timrowledge



> On Jul 19, 2014, at 12:46, "David T. Lewis" <[hidden email]> wrote:
>>
>> It's a really neat trick when developing a plugin.
>
> Yes, this mechanism does work, and it is quite feasible to distribute an
> external plugin to override an existing internal plugin. You are right
> that is it not necessary to distribute a new VM. From the point of view of
> a Squeak user who may not be familiar with how the VM is put together, it
> may be something of an academic point since we have no extablished process
> for distributing plugin updates independent of the full VM installations.
>
An excellent point. It was part of the original plan, as a way to provide bug fixes as well as flexible extensions to the vm. I seem to recall some worries about the ballooning size of the vm.... Technology has moved on since then though and even a fully bloated vm with every plugin ever written built internally is probably a small fraction of the size of typical email with yet another inline attached damn cat video.
Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] Re: [Vm-dev] Bug in writing compressed stream when saving an mcz (was: New Cog VMs available...)

Eliot Miranda-2
In reply to this post by timrowledge
 



On Sat, Jul 19, 2014 at 9:50 AM, tim Rowledge <[hidden email]> wrote:
 




It is internal in the Cog VMs.  And before you condemn that choice it /is/ slightly more efficient as it doesn't have to indirect to access the interpreterProxy API.  But in any case new Cog VMs are already available.


I know - your more direct macro access is a good idea. But that wasnt the point I intended; unless we've broken the plugin mechanism over the years a fixed-up zip plugin will over-ride the broken internal code, offering a very easy fix.


Oh *cool*!  So the loader prefers shared objects/dlls over internals if available?  Great idea.  However, it does seem to me that building and deploying a new VM is about the same complexity as building and deploying a new plugin.  In fact, slightly less, cuz installing the plugin in an existing VM is, for some, tricky.
 
It should even work in a running system where you don't want to quit and restart with a new vm for whatever reason. Something like 
Smalltalk unloadModule: 'ziplthingy'
Should disconnect from the old version and the next call to anything in the plugin ought to load the new external copy. Obviously this could be complicated by any stored state, so caveat emptor.

It's a really neat trick when developing a plugin.

 Yes indeed.

--
aloha,
Eliot
Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] Re: [Vm-dev] Bug in writing compressed stream when saving an mcz (was: New Cog VMs available...)

Eliot Miranda-2
In reply to this post by Levente Uzonyi-2
 



On Sat, Jul 19, 2014 at 11:42 AM, Levente Uzonyi <[hidden email]> wrote:

This is a good workaround for the problem, but I think the main issue is that the VM has assumptions about the layout of an object.
I would simply pass all the parameters to the primitive. There's only 14 of them, so this could work. But this plugin also writes back the state to the object, so that would create another problem.

Yes, better is an indexOfInstVarNamed: interface where the indices are computed once on first use.  But this doesn't exist as yet.

I think the best would be to create new variants of #primitiveZipSendBlock and #primitiveDeflateBlock which accept an array, and use that to load and store the paramters of the compression algorithm. This way objects with different layout would be able to use the plugin, and we could get rid of these compression streams in the long term (or change their layout without breaking them).

Best is Sista speculative inlining/adaptive optimization, where the vanilla Smalltalk code is optimized to equal or exceed C level performance.  We might be as little as two years away from that.



Another solution would be to use an external library(libraries) for compression, but that's a lot more work.


Levente

P.S.: instead of finding the number of slots of WriteStream, it would have been possible to index the slots from the end of the passed object. E.g.

loadDeflateStreamFrom: rcvr
        | slotSize oop |
        <inline: false>
        (interpreterProxy isPointers: rcvr) ifFalse: [ ^false ].
        slotSize := interpreterProxy slotSizeOf: rcvr.
        slotSize < 15 ifTrue: [ ^false ].
        ...

        zipCollection := interpreterProxy firstIndexableField: oop.
        zipCollectionSize := interpreterProxy byteSizeOf: oop.

        zipPosition := interpreterProxy fetchInteger: 1 ofObject: rcvr.
        zipReadLimit := interpreterProxy fetchInteger: 2 ofObject: rcvr.
        ...
        oop := interpreterProxy fetchPointer: slotSize - 14 ofObject: rcvr.

        ((interpreterProxy isWords: oop)
         and: [(interpreterProxy slotSizeOf: oop) = DeflateHashTableSize]) ifFalse:
                [^false].
        zipHashHead := interpreterProxy firstIndexableField: oop.
        oop := interpreterProxy fetchPointer: slotSize - 13 ofObject: rcvr.

        ((interpreterProxy isWords: oop)
         and: [(interpreterProxy slotSizeOf: oop) = DeflateWindowSize]) ifFalse:
                [^false].
        zipHashTail := interpreterProxy firstIndexableField: oop.
        zipHashValue := interpreterProxy fetchInteger: slotSize - 12 ofObject: rcvr.
        zipBlockPos := interpreterProxy fetchInteger: slotSize - 11 ofObject: rcvr.
        ...

Of course this would have its own cons.


On Sat, 19 Jul 2014, David T. Lewis wrote:


On Fri, Jul 18, 2014 at 05:26:52PM -0700, Eliot Miranda wrote:

Hi All,

   find the fix attached.  I hope we can fold this into the standard VM
soon.


Thanks Eliot, I'll update it this weekend in VMM trunk also.

Dave





--
best,
Eliot
Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] Re: [Vm-dev] Bug in writing compressed stream when saving an mcz (was: New Cog VMs available...)

David T. Lewis
In reply to this post by Eliot Miranda-2
 
On Fri, Jul 18, 2014 at 02:06:18PM -0700, Eliot Miranda wrote:

>  
> On Fri, Jul 18, 2014 at 1:51 PM, David T. Lewis <[hidden email]> wrote:
> >
> > Note that the primitive fallback code is probably still broken, but
> > perhaps that's a separate issue entirely.
> >
>
> I'm not convinced yet.  Certainly if the primitives are all removed and the
> image still fails then the fall;back code is broken.  But we need to
> determine that carefully.

To follow up on this WRT the Smalltalk that runs if the primitive fails or if
the plugin is not present:

I made a VM with no ZipPlugin, and tested with this. The failure occurs exactly
as before. So the presence of a working ZipPlugin is masking an issue in the
image. The issue appears if the plugin is absent, and it appears if the primitive
fails due the the ivar issue that we have been discussing. The problem is hidden
when a working ZipPlugin is present.

Sorry I still can't offer a test case, other than the "how to make it fail recipe"
previously discussed.

Dave

Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] Re: [Vm-dev] Bug in writing compressed stream when saving an mcz (was: New Cog VMs available...)

Eliot Miranda-2
 



On Sun, Jul 20, 2014 at 7:30 AM, David T. Lewis <[hidden email]> wrote:

On Fri, Jul 18, 2014 at 02:06:18PM -0700, Eliot Miranda wrote:
>
> On Fri, Jul 18, 2014 at 1:51 PM, David T. Lewis <[hidden email]> wrote:
> >
> > Note that the primitive fallback code is probably still broken, but
> > perhaps that's a separate issue entirely.
> >
>
> I'm not convinced yet.  Certainly if the primitives are all removed and the
> image still fails then the fall;back code is broken.  But we need to
> determine that carefully.

To follow up on this WRT the Smalltalk that runs if the primitive fails or if
the plugin is not present:

I made a VM with no ZipPlugin, and tested with this. The failure occurs exactly
as before. So the presence of a working ZipPlugin is masking an issue in the
image. The issue appears if the plugin is absent, and it appears if the primitive
fails due the the ivar issue that we have been discussing. The problem is hidden
when a working ZipPlugin is present.

 This explains the appearance of the bug in the first place.  Adding the inst vars to ReadStream and WriteStream to fix on:from:to: caused the plugin methods to fail, since values fetched from the streams were no longer as expected due to their offsets changing.  This caused the back-up code to run.

Sorry I still can't offer a test case, other than the "how to make it fail recipe"
previously discussed.

I have a suspicion.  If the plugin attempts to store a "byte" outside the 0-255 range, the least significant 8 bits get stored.  If the image attempts to store a "byte" outside the 0-255 range, the target morphs to a WideString, which will change the offsets of lots of things.
--
best,
Eliot
Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] Re: [Vm-dev] Bug in writing compressed stream when saving an mcz (was: New Cog VMs available...)

David T. Lewis
 
On Sun, Jul 20, 2014 at 11:18:18AM -0700, Eliot Miranda wrote:

>  
> On Sun, Jul 20, 2014 at 7:30 AM, David T. Lewis <[hidden email]> wrote:
>
> >
> > On Fri, Jul 18, 2014 at 02:06:18PM -0700, Eliot Miranda wrote:
> > >
> > > On Fri, Jul 18, 2014 at 1:51 PM, David T. Lewis <[hidden email]>
> > wrote:
> > > >
> > > > Note that the primitive fallback code is probably still broken, but
> > > > perhaps that's a separate issue entirely.
> > > >
> > >
> > > I'm not convinced yet.  Certainly if the primitives are all removed and
> > the
> > > image still fails then the fall;back code is broken.  But we need to
> > > determine that carefully.
> >
> > To follow up on this WRT the Smalltalk that runs if the primitive fails or
> > if
> > the plugin is not present:
> >
> > I made a VM with no ZipPlugin, and tested with this. The failure occurs
> > exactly
> > as before. So the presence of a working ZipPlugin is masking an issue in
> > the
> > image. The issue appears if the plugin is absent, and it appears if the
> > primitive
> > fails due the the ivar issue that we have been discussing. The problem is
> > hidden
> > when a working ZipPlugin is present.
>
>
>  This explains the appearance of the bug in the first place.  Adding the
> inst vars to ReadStream and WriteStream to fix on:from:to: caused the
> plugin methods to fail, since values fetched from the streams were no
> longer as expected due to their offsets changing.  This caused the back-up
> code to run.

Yes, that is exactly what happened.

>
> Sorry I still can't offer a test case, other than the "how to make it fail
> > recipe"
> > previously discussed.
>
>
> I have a suspicion.  If the plugin attempts to store a "byte" outside the
> 0-255 range, the least significant 8 bits get stored.  If the image
> attempts to store a "byte" outside the 0-255 range, the target morphs to a
> WideString, which will change the offsets of lots of things.

That may very well be the case! And this would explain why zip stream code
that has lived happily in the image for a long time now seems to have become
buggy.

Dave

Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] Re: [Vm-dev] Bug in writing compressed stream when saving an mcz (was: New Cog VMs available...)

Bert Freudenberg
 

On 20.07.2014, at 20:42, David T. Lewis <[hidden email]> wrote:

>
> On Sun, Jul 20, 2014 at 11:18:18AM -0700, Eliot Miranda wrote:
>>
>> On Sun, Jul 20, 2014 at 7:30 AM, David T. Lewis <[hidden email]> wrote:
>>
>>>
>>> On Fri, Jul 18, 2014 at 02:06:18PM -0700, Eliot Miranda wrote:
>>>>
>>>> On Fri, Jul 18, 2014 at 1:51 PM, David T. Lewis <[hidden email]>
>>> wrote:
>>>>>
>>>>> Note that the primitive fallback code is probably still broken, but
>>>>> perhaps that's a separate issue entirely.
>>>>>
>>>>
>>>> I'm not convinced yet.  Certainly if the primitives are all removed and
>>> the
>>>> image still fails then the fall;back code is broken.  But we need to
>>>> determine that carefully.
>>>
>>> To follow up on this WRT the Smalltalk that runs if the primitive fails or
>>> if
>>> the plugin is not present:
>>>
>>> I made a VM with no ZipPlugin, and tested with this. The failure occurs
>>> exactly
>>> as before. So the presence of a working ZipPlugin is masking an issue in
>>> the
>>> image. The issue appears if the plugin is absent, and it appears if the
>>> primitive
>>> fails due the the ivar issue that we have been discussing. The problem is
>>> hidden
>>> when a working ZipPlugin is present.
>>
>>
>> This explains the appearance of the bug in the first place.  Adding the
>> inst vars to ReadStream and WriteStream to fix on:from:to: caused the
>> plugin methods to fail, since values fetched from the streams were no
>> longer as expected due to their offsets changing.  This caused the back-up
>> code to run.
>
> Yes, that is exactly what happened.
>
>>
>> Sorry I still can't offer a test case, other than the "how to make it fail
>>> recipe"
>>> previously discussed.
>>
>>
>> I have a suspicion.  If the plugin attempts to store a "byte" outside the
>> 0-255 range, the least significant 8 bits get stored.  If the image
>> attempts to store a "byte" outside the 0-255 range, the target morphs to a
>> WideString, which will change the offsets of lots of things.
>
> That may very well be the case! And this would explain why zip stream code
> that has lived happily in the image for a long time now seems to have become
> buggy.
>
> Dave
>
- Bert -




smime.p7s (5K) Download Attachment
12