Fix primitiveArrayBecomeOneWayNoCopyHash

[LinqLover]

Complements VMMaker-dtl.415/Collections-eem.885. Since the latter change, Object >> #becomeForward: in Squeak 6.0Alpha has been broken in SqueakJS. As a consequence, also other important functionality such as ByteString >> #at:put: have not been working correctly.

With this change, primitive 248 (primitiveArrayBecomeOneWayNoCopyHash) is fixed again. The relevant logic was already there, just primitive 248 was still coupled to the old primitiveInvokeObjectAsMethod which is no longer up to date.

• Before (Squeak Trunk #20134):

  

• After:

  

Remaining questions:

• [ ] I could not find any pointer on when old primitiveInvokeObjectAsMethod has been used in production for the last time (has it ever been used?). At least, TestObjectAsMethodsFunction works fine in Squeak 3.8, but I really did not find any user of the old primitive in the history of Squeak. Is it sure to replace this behavior for all images that use the V3 bytecode set (see the this.oldPrims check)?

Thanks a lot to @fniephaus for the very exc{ellent,lusive,iting} introduction to the SqueakJS development! :-)

[codefrau]

I do not think this is correct. Primitive 248 was introduced specifically to not copy the hash.

Our primitiveArrayBecome defaults to copyHash = true unless that is supplied on the stack.

I would suggest to extend primitiveArrayBecome with another arg for the copyHash default. Then we call that as appropriate, e.g.

            case 72: return this.primitiveArrayBecome(argCount, false, true); // one way, copy hash
            case 128: return this.primitiveArrayBecome(argCount, true, true); // both ways, copy hash
            case 248: if (); else 
                      return this.primitiveArrayBecome(argCount, false, false); // one way, no copy hash
            case 249: return this.primitiveArrayBecome(argCount, false, true); // one way, opt. copy hash

My guess is that the tests succeed because SqueakJS does not support immutability yet. Or the test is wrong?

[LinqLover]

Good catch, thank you!

I would like to fix the tests first to have a better definition of done for this PR :-) Because #testBecomeForwardIdentityHash also fails in my latest OSVM build, I assume that the behavior was changed in Collections-eem.885 (which already states: "Switch elementsForwardIdentityTo: to not copy the hash") but the tests have not yet been updated. I have just uploaded Tests-ct.447 to the inbox and will apply your proposed changes to this PR very soon!

PS: Great, now we have a red bar which we can work with:

[LinqLover]

1c7a7e3 + Tests-ct.447

[LinqLover]

Maybe worth moving the stack access out of primitiveArrayBecome?

Looks as if the most primitive implementations are responsible themselves for accessing the stack, so I was not sure whether this change would be consistent ... 🤔

[codefrau]

@fniephaus: The argCount check guarding the stack access is infinitesimally cheaper than the actual become operation. Also, IIRC primitive 249 can be called with 1 or 2 args. I would leave it as is.

@LinqLover: Good job overall!

However, I would prefer if you changed

copyHash &&= argCount > 1 ? this.stackBoolean(argCount-2) : true;

to

if (argCount > 1) copyHash = this.stackBoolean(argCount-2);

which is a lot simpler to read.

Secondly, it would be nice to adjust the comments to explain the difference between primitives:

case 72: return this.primitiveArrayBecome(argCount, false, true); // one way, do copy hash
case 128: return this.primitiveArrayBecome(argCount, true, true); // both ways, do copy hash
case 248: if (); else 
          return this.primitiveArrayBecome(argCount, false, false); // one way, do not copy hash
case 249: return this.primitiveArrayBecome(argCount, false, true); // one way, opt. copy hash

[LinqLover]

@codefrau Thanks for the feedback! :-)

However, I would prefer if you changed

copyHash &&= argCount > 1 ? this.stackBoolean(argCount-2) : true;

to

if (argCount > 1) copyHash = this.stackBoolean(argCount-2);

which is a lot simpler to read.

Are you sure you don't mean this instead?

if (copyHash && argCount > 1) copyHash = this.stackBoolean(argCount-2);

Otherwise, we would not need the additional copyHash parameter at all ...

[codefrau]

Are you sure you don't mean this instead?

if (copyHash && argCount > 1) copyHash = this.stackBoolean(argCount-2);

Otherwise, we would not need the additional copyHash parameter at all ...

The optional argument (at argCount-2) overrides the default for copyHash, and the default comes from the primitive number.

Why wouldn't this work? It does not change the meaning of the older primitives, and the newer primitive is only ever called with one arg (but now would still work fine if called with two args).

[LinqLover]

Are you sure you don't mean this instead?

if (copyHash && argCount > 1) copyHash = this.stackBoolean(argCount-2);

Otherwise, we would not need the additional copyHash parameter at all ...

The optional argument (at argCount-2) overrides the default for copyHash, and the default comes from the primitive number.

Why wouldn't this work? It does not change the meaning of the older primitives, and the newer primitive is only ever called with one arg (but now would still work fine if called with two args).

Sure, if you don't mind that primitives 72, 248, and 249 will work identically when called with two arguments the refactoring is fine. :-)