New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Array literals implementation #173

Merged

smarr merged 5 commits into smarr:dev from unknown repository

Jul 24, 2017

ghost commented Jul 18, 2017 •

edited by smarr

Loading

This PR implements array literals, which means we can finally write down things like {1. 4*8. Random new next } at: 3 as in other languages, and do away with much of the Array with: 1 with: 4*8 verboseness.

A revamp of #100 to provide support array literals (see #83). With these changes, Newspeak's tuple literals are parsed as an ~~Generic~~ArrayLiteralNode. The node is instantiated with the list of expressions declared in the tuple, each of which are evaluated and stored into a new Object[]. If each of the expressions are evaluated to the same type, then the Object[] is replaced with a more specialised array (long[], double[], etc). The result of executing the node is an instance of SMutableArray that encapsulates the array of evaluated expressions.

Parser changes and a basic test has also been added.

ghost changed the title ~~Implement/array literals~~ Array literals implementation

smarr requested changes

View reviewed changes

Owner

smarr left a comment

Thanks a lot for updating the PR!
I got a number of things that still require some work. Some nitpicking about style consistency with the existing code, and some things that seem a bit problematic with the optimization.

Depending on your interest in this, do you want me to do these changes, or do you prefer to do them yourselves? Either way is fine for me :)

Thanks!

core-lib/TestSuite/TestRunner.som

@@ @@ -89,6 +89,7 @@ class TestRunner usingPlatform: platform = ( @@
                              'DoubleTests',
                              'IntegerTests',
                              'StringTests',
+                             'ArrayLiteralTests',

Owner

smarr Jul 18, 2017

Should also be added in https://github.com/smarr/SOMns/blob/master/tests/java/som/tests/SomTests.java#L49

Owner

smarr Jul 22, 2017

Done

core-lib/TestSuite/ArrayLiteralTests.som Outdated

+                     ].
+                     self assert: sum equals: 500500.
+                  )

Owner

smarr Jul 18, 2017

All the tests are using simple literals.
I think, we would also want tests where arrays contain complex expressions.

src/som/interpreter/nodes/literals/GenericArrayLiteralNode.java Outdated

+              public class GenericArrayLiteralNode extends LiteralNode {
+                @Children protected final ExpressionNode[] expressions;
+                public GenericArrayLiteralNode(final List<ExpressionNode> expressions, final SourceSection source) {

Owner

smarr Jul 18, 2017

I would move the conversion to an array into the parser.
It is handled there normally, or in a SNodeFactory method perhaps.

src/som/interpreter/nodes/literals/GenericArrayLiteralNode.java Outdated

+                }
+                @ExplodeLoop
+                protected SMutableArray evaluatedExpressions(final VirtualFrame frame) {

Owner

smarr Jul 18, 2017

The name seems strange to me.
why 'evaluated'?
why 'expressions'?

Doesn't really match other nodes in SOMns.
Why not simply having this in executeGeneric()?

src/som/interpreter/nodes/literals/GenericArrayLiteralNode.java Outdated

+                    arr[i] = expressions[i].executeGeneric(frame);
+                  }
+                  GenericArrayLiteralNode replacedNode = this;

Owner

smarr Jul 18, 2017

The remainder of this function is problematic.
The name of the node suggests it is the generic case, which means, it is the most general node, which is the final case of a set of specializations, usually the worst case.
So, you can't really do specializations here because it is confusing with respect to the rest of the system's conventions.

The node should probably be name UninitializedArrayLiteralNode instead, and the logic of replacing the node is typically move into a specialize() method.

src/som/interpreter/nodes/literals/GenericArrayLiteralNode.java Outdated


		GenericArrayLiteralNode replacedNode = this;

		if (allComponentsAreBoolean(arr)) {

Owner

smarr Jul 18, 2017

Hm, I am not sure this is the best way of doing it. Did you see evaluateFirstDetermineStorageAndEvaluateRest(.)? I would try to stick to existing approaches, and possibly even reuse the code.
Which might also mean that we don't need a separate class for each type of array literal.

src/som/interpreter/nodes/literals/GenericArrayLiteralNode.java Outdated

+                      try {
+                        arr[i] = expressions[i].executeBoolean(frame);
+                      } catch (UnexpectedResultException e) {
+                        throw new RuntimeException("Expression " + i + " of this BooleanArrayLiteralNode did not evaluate to a double");

Owner

smarr Jul 18, 2017

Ehm, ok?
That's not a semantically correct way to handle it :)
You can cheat, as long as you don't get caught :)

Repository owner deleted a comment

Author

ghost commented Jul 18, 2017

I've changed how the specialisation is done and now use a (simpler) specialise method instead of the separate classes.

I considered reusing behaviour from evaluateFirstDetermineStorageAndEvaluateRest as @smarr suggested, but I would need to wrap the array literal's expressions into an SBlock to do so. It seemed like more computation than was necessary, so instead I've designed the specialisation based off of that array strategy.

smarr changed the base branch from master to dev

July 20, 2017 15:32

Owner

smarr commented Jul 20, 2017

@Richard-Roberts, I took the liberty to rebase the branch on dev, which is the development branch, and the next major release.

Further, I changed the symbols to use the names from the Newspeak spec.
This is currently a little odd, and inconsistent with the rest. But, I think, we better start with something to avoid being completely inconsistent. So, { and {} are now LCurly and RCurly.

I am also thinking of squashing some of the inbetween commits, if that's ok with you.

smarr reviewed

View reviewed changes

src/som/interpreter/nodes/literals/ArrayLiteralNode.java Outdated

+                @ExplodeLoop
+                private boolean headSameTypeAsTail(final Object[] evaluated) {
+                  for (int i = 1; i < evaluated.length; i++) {
+                    if (!(evaluated[i].getClass().equals(evaluated[0].getClass()))) {

Owner

smarr Jul 20, 2017

Comparing the classes here does unfortunately not really work, since we have a wide range of classes floating around, not just primitives and objects.

smarr assigned smarr and ghost

smarr added the enhancement label

smarr added this to the v0.5.0 milestone

Owner

smarr commented Jul 20, 2017

@Richard-Roberts please have a look at my changes.
I think, I didn't explain very well what my problem was with your specialization logic.

Specializations need to form a minimal state machine, which reaches a stable point as fast as possible. Each state in the state machine is one specialization. The state transitions are defined by the 'guards'.

With my implementation, we now got a state machine which looks like this:

(Uninit)
  -> all long -> (Longs)      -\
  -> all boolean -> (Booleans) -\
  -> all double -> (Doubles)   -|-> some object -> (Objects)
  -> all nil -> (Empty)        -/
  -----------------------------/

Further, the implementation does ideally only allocate the array that is needed, so, I speculate on everything being the same type as the first element. You checked that somehow, but there was always an Object[] if I am not mistaken.

And finally, I moved the implementation into ArraySetAllStrategy since we got already more or less the same there anyway.
It's easier to keep these implementations aligned if they reside in the same file.
While they are not identical, they are pretty close.
They should be identical, but I am a little to lazy for that...
Yeah, and if Java would be a better language, we could avoid much code duplication, too.

Hope this makes somewhat sense.

smarr approved these changes

View reviewed changes

Repository owner deleted a comment

Owner

smarr commented Jul 21, 2017

I adapted code to use array literals where it makes sense.
The benchmarks look fine, too: http://somns-speed.stefan-marr.de/changes/?tre=10&rev=778c1f663758733d0accb3933abb1b2e2744440c&exe=19&env=1

Owner

smarr commented Jul 21, 2017

One more todo item for this: the dynamic metric tool needs to be taught about array literals to count their allocation

Repository owner deleted a comment

Richard Roberts and others added 5 commits

July 22, 2017 15:03


          Implement array literals conforming to current Newspeak implementations

dd65066

- these are not tuple literals as defined in the spec v0.1 sec. 5.1.7,
  instead they are mutable arrays

- the parser uses symbol names from Newspeak spec, which is inconsistent
  with the other symbol names, but we slowly should migrate to the names
  from the spec


          Added tests for array literals

31f9b76

- simple tests and complex expressions
- homogeneous and mixed types
- test specializations, and storage transitions
- added test file in runner and SomTests


          Move implementation into ArraySetAllStrategy

5219ebf

- also make sure that specialization happens only once, if possible

Signed-off-by: Stefan Marr <git@stefan-marr.de>


          Switch to array literal notation where it seems benefitial

18576e4

Signed-off-by: Stefan Marr <git@stefan-marr.de>


          Mark ArrayLiteralNode with NewArray tag

0ab4555

- also update dym test data

Signed-off-by: Stefan Marr <git@stefan-marr.de>

Owner

smarr commented Jul 22, 2017

Ok, from my perspective, this would be done and ready to be merged.
@Richard-Roberts, I pushed the changes that I'd like to merge here: https://github.com/smarr/SOMns/tree/ns-array-literals-final

The final diff is identical to the current changes on your branch, but I simplified the history to focus on the essence of the changes. Will make our life easier to figure out issues in the future.

If that all works for you, I'll merge things as soon as possible.
Thanks.

ghost commented

View reviewed changes

src/som/interpreter/nodes/literals/ArrayLiteralNode.java

+                  @Override
+                  protected Object executeSpecialized(final VirtualFrame frame) {
+                    return ArraySetAllStrategy.evalForRemaining(frame, expressions,
+                        new Object[expressions.length], SArray.FIRST_IDX);

Author

ghost Jul 23, 2017

Is there ever a case where Array.FIRST_IDX will not be zero?

Owner

smarr Jul 23, 2017

That's an artifact of my work on metaobject protocols, for those, I put some hidden things into arrays, then it wasn't 0. At the moment it is, but I kept it around for the possibility that I might need it one day. Not sure it's a great idea.

Author

ghost commented Jul 23, 2017

@smarr thanks for picking up the slack and pushing this PR along. I've looked over the changes and checked the benchmarks - let's merge this thing!

ghost closed this

ghost reopened this

Repository owner deleted a comment

smarr merged commit 4eb471c into smarr:dev

smarr mentioned this pull request

Add support for literal arrays #83

Open

ghost deleted the implement/ArrayLiterals branch

July 31, 2017 03:06

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels