I moved these two new tests from Inbox to Trunk. One test fails on recent trunk images. I do not know if this is a bug or a feature, but I cannot explain the regression so I don't want to assume it's a feature.
Dave
On Fri, Mar 29, 2013 at 02:14:02PM +0000, commits@source.squeak.org wrote:
David T. Lewis uploaded a new version of Tests to project The Trunk: http://source.squeak.org/trunk/Tests-dtl.194.mcz
==================== Summary ====================
Name: Tests-dtl.194 Author: dtl Time: 29 March 2013, 10:13:41.368 am UUID: ba96f5b8-f215-4045-a16f-473c8dde8760 Ancestors: Tests-eem.193
Merge Tests-dtl.193
Background: http://lists.squeakfoundation.org/pipermail/squeak-dev/2013-March/169911.htm...
Two tests that illustrate the limit to number of literals in a compiled method. Both tests pass on Squeak 4.4. One of the tests fails in the most recent Squeak trunk.
CompilerTest>>testMaxLiteralsWithClassReferenceInClosure passes in the image at http://build.squeak.org/job/SqueakTrunk/212/ and fails in later updates to trunk.
=============== Diff against Tests-eem.193 ===============
Item was added:
- ----- Method: CompilerTest>>testMaxLiterals (in category 'limits') -----
- testMaxLiterals
- "Document the maximum number of literals in a compiled method"
- | maxLiterals stringThatCanBeCompiled stringWithOneTooManyLiterals |
- maxLiterals := 249.
- stringThatCanBeCompiled := '{ ', (String streamContents: [:strm |
1 to: maxLiterals do: [:e | strm nextPutAll: '''', e asString, '''', ' . ']]), '}'.- stringWithOneTooManyLiterals := '{ ', (String streamContents: [:strm |
1 to: maxLiterals + 1 do: [:e | strm nextPutAll: '''', e asString, '''', ' . ']]), '}'.- self shouldnt: [Compiler evaluate: stringThatCanBeCompiled logged: false] raise: Error.
- self should: (Compiler evaluate: stringThatCanBeCompiled logged: false) size = maxLiterals.
- "If the following test fails, it means that the limit has been raised or eliminated,
- and this test should be updated to reflect the improvement."
- self should: [Compiler evaluate: stringWithOneTooManyLiterals logged: false] raise: Error.
- !
Item was added:
- ----- Method: CompilerTest>>testMaxLiteralsWithClassReferenceInClosure (in category 'limits') -----
- testMaxLiteralsWithClassReferenceInClosure
- "Document the maximum number of literals in a compiled method. A class
- reference in a closure reduces the maximum literals."
- | maxLiterals stringThatCanBeCompiled stringWithOneTooManyLiterals |
- maxLiterals := 244.
- stringThatCanBeCompiled := '[ DateAndTime now. Date today. Time ]. { ',
(String streamContents: [:strm |1 to: maxLiterals do: [:e | strm nextPutAll: '''', e asString, '''', ' . ']]), '}'.- stringWithOneTooManyLiterals := '[ DateAndTime now. Date today. Time ]. { ',
(String streamContents: [:strm |1 to: maxLiterals + 1 do: [:e | strm nextPutAll: '''', e asString, '''', ' . ']]), '}'.- self shouldnt: [Compiler evaluate: stringThatCanBeCompiled logged: false] raise: Error.
- self should: (Compiler evaluate: stringThatCanBeCompiled logged: false) size = maxLiterals.
- "If the following test fails, it means that the limit has been raised or eliminated,
- and this test should be updated to reflect the improvement."
- self should: [Compiler evaluate: stringWithOneTooManyLiterals logged: false] raise: Error.
- !
On 29 March 2013 14:17, David T. Lewis lewis@mail.msen.com wrote:
I moved these two new tests from Inbox to Trunk. One test fails on recent trunk images. I do not know if this is a bug or a feature, but I cannot explain the regression so I don't want to assume it's a feature.
Dave
http://build.squeak.org/job/SqueakTrunk/237/testReport/junit/Tests.Compiler/...
shows the test failure.
On Fri, Mar 29, 2013 at 03:04:20PM +0000, Frank Shearar wrote:
On 29 March 2013 14:17, David T. Lewis lewis@mail.msen.com wrote:
I moved these two new tests from Inbox to Trunk. One test fails on recent trunk images. I do not know if this is a bug or a feature, but I cannot explain the regression so I don't want to assume it's a feature.
Dave
http://build.squeak.org/job/SqueakTrunk/237/testReport/junit/Tests.Compiler/...
shows the test failure.
The actual change to the image that causes this test failure occurred between SqueakTrunk/212 and SqueakTrunk/213. In other words, if this test had been part of the test suite all along, then you would have seen the failure appear in 213.
I suspect that this is a side effect of class lookups associated with the Environments changes, but I cannot say if it is a bug or an explainable variation. Either way it's good to note the change, and we can either fix the bug or fix the test as needed.
Dave
On Fri, Mar 29, 2013 at 11:21:00AM -0400, David T. Lewis wrote:
On Fri, Mar 29, 2013 at 03:04:20PM +0000, Frank Shearar wrote:
On 29 March 2013 14:17, David T. Lewis lewis@mail.msen.com wrote:
I moved these two new tests from Inbox to Trunk. One test fails on recent trunk images. I do not know if this is a bug or a feature, but I cannot explain the regression so I don't want to assume it's a feature.
Dave
http://build.squeak.org/job/SqueakTrunk/237/testReport/junit/Tests.Compiler/...
shows the test failure.
The actual change to the image that causes this test failure occurred between SqueakTrunk/212 and SqueakTrunk/213. In other words, if this test had been part of the test suite all along, then you would have seen the failure appear in 213.
I suspect that this is a side effect of class lookups associated with the Environments changes, but I cannot say if it is a bug or an explainable variation. Either way it's good to note the change, and we can either fix the bug or fix the test as needed.
(self flag: #dubious and: [self flag: #bogus]) ifTrue: [self askForGuidance]
LiteralVariableNode>>sizeCodeForStore: is flagged thus, which I suspect is somehow related to the apparently incorrect warning about too many literals.
Reducing to a smaller test case, compiling 'Time now' yields a parse tree with LiteralVariableNode {Time} that has code -4 which causes dual reserving in sizeCodeForValue: and this in turn leads to a mis-counting of literals in the encoder.
In an earlier version of trunk, the same LiteralVariableNode has a non-negative code value that does not produce the dual reserving in sizeCodeForValue:
Dave
Hi David,
On Tue, Apr 2, 2013 at 7:46 PM, David T. Lewis lewis@mail.msen.com wrote:
On Fri, Mar 29, 2013 at 11:21:00AM -0400, David T. Lewis wrote:
On Fri, Mar 29, 2013 at 03:04:20PM +0000, Frank Shearar wrote:
On 29 March 2013 14:17, David T. Lewis lewis@mail.msen.com wrote:
I moved these two new tests from Inbox to Trunk. One test fails on
recent
trunk images. I do not know if this is a bug or a feature, but I
cannot
explain the regression so I don't want to assume it's a feature.
Dave
http://build.squeak.org/job/SqueakTrunk/237/testReport/junit/Tests.Compiler/...
shows the test failure.
The actual change to the image that causes this test failure occurred
between
SqueakTrunk/212 and SqueakTrunk/213. In other words, if this test had
been
part of the test suite all along, then you would have seen the failure
appear
in 213.
I suspect that this is a side effect of class lookups associated with
the Environments
changes, but I cannot say if it is a bug or an explainable variation.
Either way
it's good to note the change, and we can either fix the bug or fix the
test
as needed.
(self flag: #dubious and: [self flag: #bogus]) ifTrue: [self askForGuidance]
LiteralVariableNode>>sizeCodeForStore: is flagged thus, which I suspect is somehow related to the apparently incorrect warning about too many literals.
I doubt it. How to reproduce the incorrect warning? The point about LiteralVariableNode>>sizeCodeForStore: (& LiteralVariableNode>>emitCodeForStore:encoder:) is that they don't do what's advertised. i.e. they don't leave the value of the expression that is stored on top of stack unless the special write binding answers the expression in response to #value:. So if one were to write
v := SomeSpecialBinding := expr
v would end up holding SomeSpecialBinding, not expr.
This could perhaps be corrected by ensuring that all classes that implement isSpecialWriteBinding also implement #value: to answer the argument, not the receiver. e.g. instead of
Binding methods for accessing value: anObject (AttemptToWriteReadOnlyGlobal signal: 'Cannot store into read-only bindings') ifTrue: [value := anObject]
we use
value: anObject (AttemptToWriteReadOnlyGlobal signal: 'Cannot store into read-only bindings') ifTrue: [value := anObject]. ^anObject
but that's tricky.
The problem for the code generator is that there's no swap operation. e.g. the following would work:
stack before: expr
first half of code: dup push binding swap
stack during: expr binding expr
second half of code send #value: pop
stack after expr
but that's a) a new bytecode and b) a lot of bytes to do a store.
A different approach would be to rewrite emit because it knows the stack pointer to write something like push binding push temp N (where N is the offset of expr on the stack) send value:
I think this is the correct approach. It might require fixing the decompiler, but it shouldn't be hard. I'll have a go. In any case we need a test that ensures that
| v expr | expr := Object new. v := SomeSpecialBinding := expr. self assert: v == expr
Reducing to a smaller test case, compiling 'Time now' yields a parse tree with LiteralVariableNode {Time} that has code -4 which causes dual reserving in sizeCodeForValue: and this in turn leads to a mis-counting of literals in the encoder.
In an earlier version of trunk, the same LiteralVariableNode has a non-negative code value that does not produce the dual reserving in sizeCodeForValue:
Dave
OK, the #bogus flags are gone :) Colin write the tests and I fixed the compiler. I've also fixed the literal duplication issue (which indeed is a side-effect of introducing Environments). But in doing so the Decompiler is broken for v := Binding := expr. Kudos to anyone who can fix this (I'm busy fixing a horrible bug of my own).
On Tue, Apr 2, 2013 at 7:46 PM, David T. Lewis lewis@mail.msen.com wrote:
On Fri, Mar 29, 2013 at 11:21:00AM -0400, David T. Lewis wrote:
On Fri, Mar 29, 2013 at 03:04:20PM +0000, Frank Shearar wrote:
On 29 March 2013 14:17, David T. Lewis lewis@mail.msen.com wrote:
I moved these two new tests from Inbox to Trunk. One test fails on
recent
trunk images. I do not know if this is a bug or a feature, but I
cannot
explain the regression so I don't want to assume it's a feature.
Dave
http://build.squeak.org/job/SqueakTrunk/237/testReport/junit/Tests.Compiler/...
shows the test failure.
The actual change to the image that causes this test failure occurred
between
SqueakTrunk/212 and SqueakTrunk/213. In other words, if this test had
been
part of the test suite all along, then you would have seen the failure
appear
in 213.
I suspect that this is a side effect of class lookups associated with
the Environments
changes, but I cannot say if it is a bug or an explainable variation.
Either way
it's good to note the change, and we can either fix the bug or fix the
test
as needed.
(self flag: #dubious and: [self flag: #bogus]) ifTrue: [self askForGuidance]
LiteralVariableNode>>sizeCodeForStore: is flagged thus, which I suspect is somehow related to the apparently incorrect warning about too many literals.
Reducing to a smaller test case, compiling 'Time now' yields a parse tree with LiteralVariableNode {Time} that has code -4 which causes dual reserving in sizeCodeForValue: and this in turn leads to a mis-counting of literals in the encoder.
In an earlier version of trunk, the same LiteralVariableNode has a non-negative code value that does not produce the dual reserving in sizeCodeForValue:
Dave
Yay! And CompilerTest>>testMaxLiteralsWithClassReferenceInClosure is green now :)
Don't get me wrong, I really appreciated those #bogus flags. Seriously. It was an excellent clue that I should stop random debugging and ask for help from someone who might actually know what they were doing ;-)
Dave
On Wed, Apr 03, 2013 at 03:34:59PM -0700, Eliot Miranda wrote:
OK, the #bogus flags are gone :) Colin write the tests and I fixed the compiler. I've also fixed the literal duplication issue (which indeed is a side-effect of introducing Environments). But in doing so the Decompiler is broken for v := Binding := expr. Kudos to anyone who can fix this (I'm busy fixing a horrible bug of my own).
On Tue, Apr 2, 2013 at 7:46 PM, David T. Lewis lewis@mail.msen.com wrote:
On Fri, Mar 29, 2013 at 11:21:00AM -0400, David T. Lewis wrote:
On Fri, Mar 29, 2013 at 03:04:20PM +0000, Frank Shearar wrote:
On 29 March 2013 14:17, David T. Lewis lewis@mail.msen.com wrote:
I moved these two new tests from Inbox to Trunk. One test fails on
recent
trunk images. I do not know if this is a bug or a feature, but I
cannot
explain the regression so I don't want to assume it's a feature.
Dave
http://build.squeak.org/job/SqueakTrunk/237/testReport/junit/Tests.Compiler/...
shows the test failure.
The actual change to the image that causes this test failure occurred
between
SqueakTrunk/212 and SqueakTrunk/213. In other words, if this test had
been
part of the test suite all along, then you would have seen the failure
appear
in 213.
I suspect that this is a side effect of class lookups associated with
the Environments
changes, but I cannot say if it is a bug or an explainable variation.
Either way
it's good to note the change, and we can either fix the bug or fix the
test
as needed.
(self flag: #dubious and: [self flag: #bogus]) ifTrue: [self askForGuidance]
LiteralVariableNode>>sizeCodeForStore: is flagged thus, which I suspect is somehow related to the apparently incorrect warning about too many literals.
Reducing to a smaller test case, compiling 'Time now' yields a parse tree with LiteralVariableNode {Time} that has code -4 which causes dual reserving in sizeCodeForValue: and this in turn leads to a mis-counting of literals in the encoder.
In an earlier version of trunk, the same LiteralVariableNode has a non-negative code value that does not produce the dual reserving in sizeCodeForValue:
Dave
-- best, Eliot
squeak-dev@lists.squeakfoundation.org
-
David T. Lewis -
Eliot Miranda -
Frank Shearar