On Sun, Sep 7, 2014 at 1:58 PM, commits@source.squeak.org wrote:
David T. Lewis uploaded a new version of System to project The Trunk: http://source.squeak.org/trunk/System-dtl.685.mcz
==================== Summary ====================
Name: System-dtl.685 Author: dtl Time: 7 September 2014, 4:58:15.377 pm UUID: 55eba121-23d3-41cc-9525-52554adac2ec Ancestors: System-eem.684
Smalltalk isRunningCog should answer false for an interpreter VM
=============== Diff against System-eem.684 ===============
Item was changed: ----- Method: SmalltalkImage>>isRunningCog (in category 'system attributes') ----- isRunningCog "Answers if we're running on a Cog VM (JIT or StackInterpreter)"
^( [self vmParameterAt: 42] on: Error do: [] )
^(self vmParameterAt: 42) ifNil: [false] ifNotNil: [:numStackPages| numStackPages > 0]!
Instead of creating an exception handler why not modify the interpreter to answer nil? I know the answer is that the Interpreter doesn't, and fails. But we've had years in which we could change the Interpreter to match vmParameterAt: and instead of doing the good thing (have the Interpreter answer nil for values answered by Cog VMs), we force the standard system to waste time. We should optimize the common case (trunk running on Cog) not penalize it to cover the uncommon case (trunk running on Interpreter VM).
On 08.09.2014, at 20:45, Eliot Miranda eliot.miranda@gmail.com wrote:
On Sun, Sep 7, 2014 at 1:58 PM, commits@source.squeak.org wrote: David T. Lewis uploaded a new version of System to project The Trunk: http://source.squeak.org/trunk/System-dtl.685.mcz
==================== Summary ====================
Name: System-dtl.685 Author: dtl Time: 7 September 2014, 4:58:15.377 pm UUID: 55eba121-23d3-41cc-9525-52554adac2ec Ancestors: System-eem.684
Smalltalk isRunningCog should answer false for an interpreter VM
=============== Diff against System-eem.684 ===============
Item was changed: ----- Method: SmalltalkImage>>isRunningCog (in category 'system attributes') ----- isRunningCog "Answers if we're running on a Cog VM (JIT or StackInterpreter)"
^( [self vmParameterAt: 42] on: Error do: [] )
^(self vmParameterAt: 42) ifNil: [false] ifNotNil: [:numStackPages| numStackPages > 0]!
Instead of creating an exception handler why not modify the interpreter to answer nil? I know the answer is that the Interpreter doesn't, and fails. But we've had years in which we could change the Interpreter to match vmParameterAt: and instead of doing the good thing (have the Interpreter answer nil for values answered by Cog VMs), we force the standard system to waste time. We should optimize the common case (trunk running on Cog) not penalize it to cover the uncommon case (trunk running on Interpreter VM).
Nah, if performance really was an issue we could just remove the primitiveFailed in vmParameterAt:. No need to change the VM.
- Bert -
I'm not one to tread into you VM guys' domain but I just wanted to express my agreement with Eliot. "Performance" isn't the only viable criticism of using on: Error do: [ ... ]. It's just plain bad style, bad code. We know better than to do that.
On Mon, Sep 8, 2014 at 1:58 PM, Bert Freudenberg bert@freudenbergs.de wrote:
On 08.09.2014, at 20:45, Eliot Miranda eliot.miranda@gmail.com wrote:
On Sun, Sep 7, 2014 at 1:58 PM, commits@source.squeak.org wrote:
David T. Lewis uploaded a new version of System to project The Trunk: http://source.squeak.org/trunk/System-dtl.685.mcz
==================== Summary ====================
Name: System-dtl.685 Author: dtl Time: 7 September 2014, 4:58:15.377 pm UUID: 55eba121-23d3-41cc-9525-52554adac2ec Ancestors: System-eem.684
Smalltalk isRunningCog should answer false for an interpreter VM
=============== Diff against System-eem.684 ===============
Item was changed: ----- Method: SmalltalkImage>>isRunningCog (in category 'system attributes') ----- isRunningCog "Answers if we're running on a Cog VM (JIT or StackInterpreter)"
^( [self vmParameterAt: 42] on: Error do: [] )
^(self vmParameterAt: 42) ifNil: [false] ifNotNil: [:numStackPages| numStackPages > 0]!
Instead of creating an exception handler why not modify the interpreter to answer nil? I know the answer is that the Interpreter doesn't, and fails. But we've had years in which we could change the Interpreter to match vmParameterAt: and instead of doing the good thing (have the Interpreter answer nil for values answered by Cog VMs), we force the standard system to waste time. We should optimize the common case (trunk running on Cog) not penalize it to cover the uncommon case (trunk running on Interpreter VM).
Nah, if performance really was an issue we could just remove the primitiveFailed in vmParameterAt:. No need to change the VM.
- Bert -
A method that tests for "is running Cog" and that blows up if you are not running Cog is not a very sensible thing. If you can make it faster and prettier that's great, but please do check to make sure it actually works.
Dave
I'm not one to tread into you VM guys' domain but I just wanted to express my agreement with Eliot. "Performance" isn't the only viable criticism of using on: Error do: [ ... ]. It's just plain bad style, bad code. We know better than to do that.
On Mon, Sep 8, 2014 at 1:58 PM, Bert Freudenberg bert@freudenbergs.de wrote:
On 08.09.2014, at 20:45, Eliot Miranda eliot.miranda@gmail.com wrote:
On Sun, Sep 7, 2014 at 1:58 PM, commits@source.squeak.org wrote:
David T. Lewis uploaded a new version of System to project The Trunk: http://source.squeak.org/trunk/System-dtl.685.mcz
==================== Summary ====================
Name: System-dtl.685 Author: dtl Time: 7 September 2014, 4:58:15.377 pm UUID: 55eba121-23d3-41cc-9525-52554adac2ec Ancestors: System-eem.684
Smalltalk isRunningCog should answer false for an interpreter VM
=============== Diff against System-eem.684 ===============
Item was changed: ----- Method: SmalltalkImage>>isRunningCog (in category 'system attributes') ----- isRunningCog "Answers if we're running on a Cog VM (JIT or StackInterpreter)"
^( [self vmParameterAt: 42] on: Error do: [] )
^(self vmParameterAt: 42) ifNil: [false] ifNotNil: [:numStackPages| numStackPages > 0]!
Instead of creating an exception handler why not modify the interpreter to answer nil? I know the answer is that the Interpreter doesn't, and fails. But we've had years in which we could change the Interpreter to match vmParameterAt: and instead of doing the good thing (have the Interpreter answer nil for values answered by Cog VMs), we force the standard system to waste time. We should optimize the common case (trunk running on Cog) not penalize it to cover the uncommon case (trunk running on Interpreter VM).
Nah, if performance really was an issue we could just remove the primitiveFailed in vmParameterAt:. No need to change the VM.
- Bert -
On Mon, Sep 8, 2014 at 11:58 AM, Bert Freudenberg bert@freudenbergs.de wrote:
On 08.09.2014, at 20:45, Eliot Miranda eliot.miranda@gmail.com wrote:
On Sun, Sep 7, 2014 at 1:58 PM, commits@source.squeak.org wrote:
David T. Lewis uploaded a new version of System to project The Trunk: http://source.squeak.org/trunk/System-dtl.685.mcz
==================== Summary ====================
Name: System-dtl.685 Author: dtl Time: 7 September 2014, 4:58:15.377 pm UUID: 55eba121-23d3-41cc-9525-52554adac2ec Ancestors: System-eem.684
Smalltalk isRunningCog should answer false for an interpreter VM
=============== Diff against System-eem.684 ===============
Item was changed: ----- Method: SmalltalkImage>>isRunningCog (in category 'system attributes') ----- isRunningCog "Answers if we're running on a Cog VM (JIT or StackInterpreter)"
^( [self vmParameterAt: 42] on: Error do: [] )
^(self vmParameterAt: 42) ifNil: [false] ifNotNil: [:numStackPages| numStackPages > 0]!
Instead of creating an exception handler why not modify the interpreter to answer nil? I know the answer is that the Interpreter doesn't, and fails. But we've had years in which we could change the Interpreter to match vmParameterAt: and instead of doing the good thing (have the Interpreter answer nil for values answered by Cog VMs), we force the standard system to waste time. We should optimize the common case (trunk running on Cog) not penalize it to cover the uncommon case (trunk running on Interpreter VM).
Nah, if performance really was an issue we could just remove the primitiveFailed in vmParameterAt:. No need to change the VM.
That's a good idea. Change the primitiveFailed to ^nil.
On Mon, Sep 08, 2014 at 01:09:17PM -0700, Eliot Miranda wrote:
On Mon, Sep 8, 2014 at 11:58 AM, Bert Freudenberg bert@freudenbergs.de wrote:
On 08.09.2014, at 20:45, Eliot Miranda eliot.miranda@gmail.com wrote:
Instead of creating an exception handler why not modify the interpreter to answer nil? I know the answer is that the Interpreter doesn't, and fails. But we've had years in which we could change the Interpreter to match vmParameterAt: and instead of doing the good thing (have the Interpreter answer nil for values answered by Cog VMs), we force the standard system to waste time. We should optimize the common case (trunk running on Cog) not penalize it to cover the uncommon case (trunk running on Interpreter VM).
All of the VMs, including Cog, behave identically in this regard. If you request a VM parameter that is not understood by the VM, the primitive fails.
Nah, if performance really was an issue we could just remove the primitiveFailed in vmParameterAt:. No need to change the VM.
That's a good idea. Change the primitiveFailed to ^nil.
It would be a good idea if performance was actually an issue. It is not.
This is a case where using exceptions makes good sense. If some arbitrary VM cannot satisfy a primitive request, it should fail the primitive. Answering nil as a flag for primitive failed is bad karma, and should not be done unless there is a good reason for doing so. Improving the performance of #isRunningCog is not a good reason.
BTW there is another method with the same problem:
isRunningCogit "Answers if we're running on the Cog JIT"
There are no senders of #isRunningCogit in the image, so it does not need to be fixed. But the #isRunningCog bug was breaking the unit tests, so it needed fixing.
Dave
On 09.09.2014, at 04:49, David T. Lewis lewis@mail.msen.com wrote:
On Mon, Sep 08, 2014 at 01:09:17PM -0700, Eliot Miranda wrote:
On Mon, Sep 8, 2014 at 11:58 AM, Bert Freudenberg bert@freudenbergs.de wrote:
On 08.09.2014, at 20:45, Eliot Miranda eliot.miranda@gmail.com wrote:
Instead of creating an exception handler why not modify the interpreter to answer nil? I know the answer is that the Interpreter doesn't, and fails. But we've had years in which we could change the Interpreter to match vmParameterAt: and instead of doing the good thing (have the Interpreter answer nil for values answered by Cog VMs), we force the standard system to waste time. We should optimize the common case (trunk running on Cog) not penalize it to cover the uncommon case (trunk running on Interpreter VM).
All of the VMs, including Cog, behave identically in this regard. If you request a VM parameter that is not understood by the VM, the primitive fails.
Nah, if performance really was an issue we could just remove the primitiveFailed in vmParameterAt:. No need to change the VM.
That's a good idea. Change the primitiveFailed to ^nil.
It would be a good idea if performance was actually an issue. It is not.
This is a case where using exceptions makes good sense. If some arbitrary VM cannot satisfy a primitive request, it should fail the primitive. Answering nil as a flag for primitive failed is bad karma, and should not be done unless there is a good reason for doing so. Improving the performance of #isRunningCog is not a good reason.
BTW there is another method with the same problem:
isRunningCogit "Answers if we're running on the Cog JIT"
There are no senders of #isRunningCogit in the image, so it does not need to be fixed. But the #isRunningCog bug was breaking the unit tests, so it needed fixing.
Dave
+1
- Bert -
squeak-dev@lists.squeakfoundation.org