Skip to content

Conversation

@daumayr
Copy link
Contributor

@daumayr daumayr commented Feb 13, 2019

This PR implements replay from snapshots.
It also fixes various problems with the previous snapshot implementation e.g. Outer Objects of Classses, ownership of promises and various duplication issues.

TODO: enable snapshotting/serialization tests

The result promise is serialised in another actor and therefore serialised as completed.
previously returns caused a lack of progress issue.
correct the specialization order, remove magic numbers, avoid serializing delivered messages on the wrong actor.
correct the handling of Promises, add static deserialisation option.
When the assumption becomes false while specialising, an UnsupportedSpecializationException was caused because no fallback existed.
// need to be careful, might interfere with promise serialization...
return -1;
@TruffleBoundary
// TODO: can we establish a structure for this? at the moment, we have an
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now handled, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The serializer is compilation final (child node of receive).
There is still some indirection of accessing fields

this.target = target;

if (VmSettings.SNAPSHOTS_ENABLED) {
if (VmSettings.SNAPSHOTS_ENABLED && !VmSettings.REPLAY) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing these and other flags, should we try to have two distinct: SNAPSHOT_RECORDING and SNAPSHOT_REPLAY? This could possibly avoid issues with wrongly combining/testing flags?

So, I mean, instead of having SNAPSHOT_ENABLED.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, refactoring the flags probably reduces ambiguity and improves readability of the code

this.isArray = isArray;
this.instanceClassGroup = classFactory;

if (VmSettings.SNAPSHOTS_ENABLED) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this change go into its own method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i can make an initializeForSnapshotting method

}

public void handleTodos(final SnapshotBuffer sb) {
@TruffleBoundary // TODO: convert to an approach that constructs a cache
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this TODO still current?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only if you want to use cachedserialization nodes

return deserialized.size();
}

private void printPosition(final long current) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and the printClass methods look strange.
Are they used? Should they be implemented but have a VmSettings.DEBUG flag or something? I think there may be such a flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't used them for a while, we can probably remove them.
We could also use a flag, but this can produce a LOT of output.

@smarr smarr added the research Work part of a research project label Aug 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

research Work part of a research project

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants