Skip to content
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.

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

Serialization Improvements #276

Merged
merged 9 commits into from Oct 30, 2018

Conversation

2 participants
@daumayr
Copy link
Contributor

commented Oct 23, 2018

This PR is another step towards actor snapshots, it introduces:

  • Support for deserializing circular object graphs by introducing a fixup system:
    When a not yet available object is referenced, we temporarily assign a placeholder (e.g. null), and create a FixupInformation object that allows us to insert the correct reference once the object is available (deserialized).

  • Serialization of blocks:
    Blocks can be (de)serialized, including their context (MaterializedFrame).
    We do not serialize FrameDescriptors and instead recover that information in replay from the blocks invokable.
    Non-local returns from deserialized blocks are currently not supported (this means we cover sufficient semantics of for instance JS).

  • Serialization of Promises and Messages:
    Promises including any scheduled callbacks, messages, or chained promises can be (de)serialized.
    Message serialization will be essential for snapshotting, as it will be our starting point for snapshots (i.e. serializing all objects that are reachable from serialized messages).

@smarr
Copy link
Owner

left a comment

Thanks for the PR.

And yes, this was really necessary, I think.
We are well above 1000 LOC change again.

This all is rather more complex than I had hoped :(

ifNil: [ TestError case: self exception: ex ]
ifNotNil: [:it | it createErrorResultFor: self exception: ex ])
ifNil: [ TestError case: self exception: e ]
ifNotNil: [:it | it createErrorResultFor: self exception: e ])

This comment has been minimized.

Copy link
@smarr

smarr Oct 23, 2018

Owner

Why did you do the renamings in this file? Seems unnecessary, and unrelated to what you want to achieve in this PR.

This comment has been minimized.

Copy link
@daumayr

daumayr Oct 24, 2018

Author Contributor

There is no ex variable in that scope, only e. Instead of fixing the usage I can rename e to ex if you prefer.

This comment has been minimized.

Copy link
@smarr

smarr Oct 24, 2018

Owner

Ahhhh. No, the change is good. My comment just shows that it would help me to get more context for such changes (possibly by separate maintenance PRs, added tests, ...)

I mean, it's already good that you have a separate commit for the timeout stuff. But it's unfortunate that it doesn't come with any testing, any any comment for the rational for the changes.

]
onError: [:actualResolution |
failWithMessage: 'Promise broken with error: ' + actualResolution asString ].
^ assertPP promise.

This comment has been minimized.

Copy link
@smarr

smarr Oct 23, 2018

Owner

What are all theses changes about?
This looks problematic to me.
And it doesn't seem to be a self-contained and documented change either.
The time out is rather arbitrary, too.

If this is needed, or desired, could you please break this change out?
And would be good to have an assert:resolvedWith:timeout: which takes an explicit parameter.
assert:resolvedWith: can then call it explicitly.

This comment has been minimized.

Copy link
@daumayr

daumayr Oct 24, 2018

Author Contributor

The current assert:resolvedWith: ignores the case where the promise is not resolved. I'll remove the change from this PR and put it into a Separate one with a version for specifying timeout.

This comment has been minimized.

Copy link
@smarr

smarr Oct 24, 2018

Owner

Thank you.

This comment has been minimized.

Copy link
@smarr

smarr Oct 24, 2018

Owner

Just for an explanation: I didn't actually notice that this was in a separate commit. Missed that yesterday.

@@ -20,6 +20,7 @@ THE SOFTWARE.
*)
class SerializationTests usingPlatform: platform testFramework: minitest = Value(
| private TestContext = minitest TestContext.

This comment has been minimized.

Copy link
@smarr

smarr Oct 23, 2018

Owner

Do you still need the TestContext?

private final SSymbol selector;
protected Actor target;
protected Actor finalSender;
protected SPromise originalTarget;

This comment has been minimized.

Copy link
@smarr

smarr Oct 23, 2018

Owner

Could you at least make it @CompilationFinal then? This is likely a performance regression.

@@ -281,7 +308,7 @@ protected PromiseSendMessage(final SSymbol selector, final Object[] arguments,
/**
* The promise on which this callback is registered on.
*/
protected final SPromise promise;
protected SPromise promise;

This comment has been minimized.

Copy link
@smarr

smarr Oct 23, 2018

Owner

Could you at least make it @CompilationFinal then? This is likely a performance regression.

Show resolved Hide resolved src/tools/snapshot/nodes/MessageSerializationNode.java
Show resolved Hide resolved src/tools/snapshot/nodes/PromiseSerializationNodes.java
Show resolved Hide resolved src/tools/snapshot/nodes/PromiseSerializationNodes.java
}
}

// Resolvers are values and can be passed directly without being wrapped

This comment has been minimized.

Copy link
@smarr

smarr Oct 23, 2018

Owner

Should this be a JavaDoc comment on the class?

// From what i have seen there is only one resolver created for any promise.
// Resolver contains a reference to the promise, which knows it's owner.
// If Identity of Resolvers becomes an issue just add the actor information and turn into a
// singleton when deserializing

This comment has been minimized.

Copy link
@smarr

smarr Oct 23, 2018

Owner

There is only a single resolver for every promise. But promises may be chained.

@daumayr daumayr force-pushed the daumayr:Messages branch from 22bba47 to 8e4cd2e Oct 24, 2018

@@ -1238,6 +1238,9 @@ private ExpressionNode primary(final MethodBuilder builder) throws ProgramDefini
SInvokable blockMethod = bgenc.assemble(blockBody,
AccessModifier.BLOCK_METHOD, lastMethodsSourceSection);
builder.addEmbeddedBlockMethod(blockMethod);
if (structuralProbe != null && VmSettings.TRACK_SNAPSHOT_ENTITIES) {

This comment has been minimized.

Copy link
@daumayr

daumayr Oct 24, 2018

Author Contributor

probably should switch order of conditions (not sure if this gets optimized)

Repository owner deleted a comment from codacy-bot Oct 24, 2018

Repository owner deleted a comment from codacy-bot Oct 24, 2018

Repository owner deleted a comment from codacy-bot Oct 25, 2018

@smarr smarr added the enhancement label Oct 25, 2018

@smarr smarr added this to To Do in Actor Record & Replay via automation Oct 25, 2018

@smarr smarr added this to the v0.7.0 milestone Oct 25, 2018

@smarr smarr moved this from To Do to In Progress in Actor Record & Replay Oct 25, 2018

Repository owner deleted a comment from codacy-bot Oct 25, 2018

Repository owner deleted a comment from codacy-bot Oct 25, 2018

Repository owner deleted a comment from codacy-bot Oct 25, 2018

@smarr

smarr approved these changes Oct 30, 2018

Copy link
Owner

left a comment

Thanks for the changes! looks good to me!

@smarr smarr force-pushed the daumayr:Messages branch from 3bb2d8e to 4ef30d7 Oct 30, 2018

@smarr

This comment has been minimized.

Copy link
Owner

commented Oct 30, 2018

Going to merge once all tests completed, and the benchmarks ran.

@smarr smarr merged commit 43e76e1 into smarr:dev Oct 30, 2018

1 of 2 checks passed

Codacy/PR Quality Review Hang in there, Codacy is reviewing your Pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Actor Record & Replay automation moved this from In Progress to Done Oct 30, 2018

@daumayr daumayr deleted the daumayr:Messages branch Oct 31, 2018

@smarr

This comment has been minimized.

Dominik, could we find a better name for this? What are the things serialized here?

This comment has been minimized.

Copy link
Owner Author

replied Dec 18, 2018

we serialize message type, selector, and the actorId of the original sender

This comment has been minimized.

Copy link

replied Dec 18, 2018

How about: serializeMessageMetaData?

This comment has been minimized.

Copy link
Owner Author

replied Dec 18, 2018

ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.