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

Basic Object Serialisation #271

Merged
merged 31 commits into from Oct 15, 2018

Conversation

@daumayr
Copy link
Contributor

commented Sep 26, 2018

This PR is the first step towards introducing actor snapshots to SOMns,
it adds basic (de)serializing operations and infrastructure that will be needed
in the creation of snapshots.
Currently serialisation is limited to acyclic object graphs without duplicates (i.e. a tree).

Each SClass now has a serialization node, which can be used to (de)serialize instances of the represented class.
Tests are based on cloning objects by serialisation & deserialisation.

Possibly introduces a performance regression for the startup time of the List benchmark on Graal (#273)


protected void serializeStorage(final SArray sa, final SnapshotBuffer sb) {
int requiredSpace;
if (sa.isBooleanType()) {

This comment has been minimized.

Copy link
@smarr

smarr Sep 26, 2018

Owner

Since this looks like a node, shouldn't it simply specialize on the array type with a @Specialization and guard?

Repository owner deleted a comment from codacy-bot Sep 26, 2018

@Override
public void serialize(final Object o, final SnapshotBuffer sb) {
assert o instanceof SObject;
((SObject) o).updateLayoutToMatchClass();

This comment has been minimized.

Copy link
@smarr

smarr Sep 26, 2018

Owner

This seems to be something that should be done conditionally, as part of specialization.

Also, this should really be done in a safepoint, see https://github.com/smarr/SOMns/blob/dev/src/som/interpreter/nodes/dispatch/UninitializedDispatchNode.java#L160


if (!sb.containsObject(value)) {
// Referenced Object not yet in snapshot
SClass c = Types.getClassOf(value);

This comment has been minimized.

Copy link
@smarr

smarr Sep 26, 2018

Owner

For performance, we likely want to start caching these things.

Show resolved Hide resolved build.xml

@daumayr daumayr force-pushed the daumayr:Actor-Snapshots branch from d9af478 to d66953c Sep 27, 2018

@daumayr daumayr force-pushed the daumayr:Actor-Snapshots branch 3 times, most recently from 965373c to a06edf3 Sep 27, 2018

@daumayr daumayr force-pushed the daumayr:Actor-Snapshots branch 2 times, most recently from 98ff0cd to 374a44d Sep 27, 2018

@daumayr daumayr force-pushed the daumayr:Actor-Snapshots branch from 374a44d to b5e7da5 Sep 28, 2018

@smarr
Copy link
Owner

left a comment

I think generally fine, there are some cosmetics in here, I'd like you to look at.

Otherwise, I am happy to merge it without anything extra. Though, I doubt this code will survive the first benchmark unchanged.

vmMirror snapshot: obj.
)

public sclone: obj = (

This comment has been minimized.

Copy link
@smarr

smarr Sep 29, 2018

Owner

No abbreviations please.

|)(

public class ValueClass new: a and: b = Value (
|

This comment has been minimized.

Copy link
@smarr

smarr Sep 29, 2018

Owner

Inconsistent whitespace format for fields.

)

public class SerializationTest = TestContext (
|

This comment has been minimized.

Copy link
@smarr

smarr Sep 29, 2018

Owner

Inconsistent whitespace format for fields.

clone:: Actors sclone: original.
original println.
clone println.
assert: original == original.

This comment has been minimized.

Copy link
@smarr

smarr Sep 29, 2018

Owner

#assert:is: would be better here, I think.

Also, shouldn't this be comparing original to clone?

| original clone |
original:: SerializationTest.
clone:: Actors sclone: original.
original println.

This comment has been minimized.

Copy link
@smarr

smarr Sep 29, 2018

Owner

there shouldn't be prints in tests.

public class SnapshotBackend {
private static byte snapshotVersion = 0;

private static EconomicMap<Short, SSymbol> symbolDictionary;

This comment has been minimized.

Copy link
@smarr

smarr Sep 29, 2018

Owner

should these be final?


// This map allows us to know if we already serialized an object (and avoid circles)
// We can get the location of the serialized object in the trace
private EconomicMap<Object, Long> entries;

This comment has been minimized.

Copy link
@smarr

smarr Sep 29, 2018

Owner

fields are defined before the constructor.

Should this be final?


@Override
protected void swapBufferWhenNotEnoughSpace(final TraceActorContextNode tracer) {
throw new UnsupportedOperationException();

This comment has been minimized.

Copy link
@smarr

smarr Sep 29, 2018

Owner

UnsupportedOperationException(msg) takes a string. Would be better to put what you have in the comment as a string there.

@Children private CachedSlotRead[] fieldReads;
@Children private CachedSlotWrite[] fieldWrites;
@Children private CachedSerializationNode[] cachedSerializers;
protected ObjectLayout layout;

This comment has been minimized.

Copy link
@smarr

smarr Sep 29, 2018

Owner

should this be final?

CachedSlotRead[] newChildren = createReadNodes(so);

for (int i = 0; i < fieldCnt; i++) {
fieldReads[i].replace(newChildren[i]);

This comment has been minimized.

Copy link
@smarr

smarr Sep 29, 2018

Owner

this looks brittle and I can't tell whether this even works.

I was wondering whether simply replacing the full node (this) would be saver?

@smarr

This comment has been minimized.

Copy link
Owner

commented on 60c2188 Oct 1, 2018

I don't really understand the change. Perhaps a description of the intention as part of the commit message could have helped me?

@smarr

This comment has been minimized.

Copy link
Owner

commented on core-lib/TestSuite/Serialization.ns in 794e56d Oct 1, 2018

You are adding spurious whitespace?
Could you change your editor to automatically remove these things?

@smarr

This comment has been minimized.

Copy link
Owner

commented on src/tools/snapshot/SnapshotBuffer.java in 794e56d Oct 1, 2018

Just seeing this. Why is the comment not done as JavaDoc? works much better with tools and IDE support if it is in:

/**
  *
  */

daumayr added some commits Oct 1, 2018

@daumayr daumayr force-pushed the daumayr:Actor-Snapshots branch from 6735ea1 to 3e37873 Oct 3, 2018

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

@smarr
Copy link
Owner

left a comment

Here a bunch of other cosmetic issues.

And there are still other things you haven't respondent to, or changed.
Would be good if we could at least discuss them.

| public f1 = a.
public f2 = b.|
)(
public verify: clone =(

This comment has been minimized.

Copy link
@smarr

smarr Oct 3, 2018

Owner

This isn't used, is it? Do we need it?
con is not defined.

Show resolved Hide resolved core-lib/TestSuite/Serialization.ns
Show resolved Hide resolved core-lib/TestSuite/Serialization.ns
*)
class SerializationTests usingPlatform: platform testFramework: minitest = Value(
| private TestContext = minitest TestContext.
private Actors = platform actors.

This comment has been minimized.

Copy link
@smarr

smarr Oct 3, 2018

Owner

This is a module instance, please do not capitalize.
Only classes are capitalize.

class SerializationTests usingPlatform: platform testFramework: minitest = Value(
| private TestContext = minitest TestContext.
private Actors = platform actors.
private String = platform kernel String.

This comment has been minimized.

Copy link
@smarr

smarr Oct 3, 2018

Owner

Why the strange white space here? Inconsistent with the rest.

* serialize
* objects on demand.
*
* @author dominikaumayr

This comment has been minimized.

Copy link
@smarr
}
return 0;
}
return 0;

This comment has been minimized.

Copy link
@smarr

short cId = bb.getShort();

SClass clazz = SnapshotBackend.lookupClass(cId);

This comment has been minimized.

Copy link
@smarr
@@ -66,26 +76,50 @@

@CompilationFinal private ClassFactory instanceClassGroup; // the factory for this object

@CompilationFinal protected SerializerRootNode serializationRoot;

This comment has been minimized.

Copy link
@smarr

smarr Oct 3, 2018

Owner

This should be final now, no?

}

public AbstractSerializationNode getSerializer() {
return ((SerializerRootNode) serializationRoot.getRootNode()).getSerializer();

This comment has been minimized.

Copy link
@smarr

smarr Oct 3, 2018

Owner

This should be serializationRoot.getSerializer() no?

@daumayr daumayr force-pushed the daumayr:Actor-Snapshots branch from 03d52c3 to f121b69 Oct 5, 2018

public String getIdentifier() {
MixinDefinition outer = getOuterMixinDefinition();
if (outer != null) {
return outer.getIdentifier() + "." + this.name.getString();
} else if (this.isModule && this.sourceSection != null) {
return this.sourceSection.getSource().getPath() + ":" + this.name.getString();
Path absolute = Paths.get(this.sourceSection.getSource().getURI());
Path relative = Paths.get("").toAbsolutePath().relativize(absolute);

This comment has been minimized.

Copy link
@smarr

smarr Oct 5, 2018

Owner

This is going to change when the working directory is changed by the program, right?

This comment has been minimized.

Copy link
@daumayr

daumayr Oct 5, 2018

Author Contributor

Right, do we have the SOMns path stored somewhere?

This comment has been minimized.

Copy link
@smarr

smarr Oct 5, 2018

Owner

Not really, I think. There are various things passed in as -D flags or parameter in the som launcher script. For instance the kernel file, platform file, and tools directory. Perhaps we want the core-lib folder as relative root, too?

This comment has been minimized.

Copy link
@daumayr

daumayr Oct 5, 2018

Author Contributor

We don't need a new parameter for this, we can just use the directory containing the kernel file.

this.serializationRoot = new SerializerRootNode(null, factory.createNode(this));
if (VmSettings.SNAPSHOTS_ENABLED) {
CompilerDirectives.transferToInterpreter();
this.serializationRoot = new SerializerRootNode(null, factory.createNode(this));

This comment has been minimized.

Copy link
@smarr

smarr Oct 5, 2018

Owner

The transferToInterpreter makes me wonder whether the serializationRoot shouldn't go into the instanceClassGroup.
Also, please avoid the generic factory name for the parameter, it's confusing in this context.

This comment has been minimized.

Copy link
@daumayr

daumayr Oct 5, 2018

Author Contributor

Moving the serializationRoot shouldn't be too much work. Do you wan't me to move it, or add a comment/TODO and consider it later.

This comment has been minimized.

Copy link
@smarr

smarr Oct 5, 2018

Owner

Conceptually it seems to belong into instanceClassGroup. SClass should really just provide identity. Everything else should be in the more stable structures. We don't want a separate serializer per instance of SClass either, I think.

This comment has been minimized.

Copy link
@daumayr

daumayr Oct 5, 2018

Author Contributor

From what i saw ObjectLiterals create a new SClass for each instance. Moving the serializer would be beneficial in that case, and ensure there is only one.

This comment has been minimized.

Copy link
@smarr

smarr Oct 5, 2018

Owner

Not just object literals, any nested class is potentially instantiated many times.
Inner classes for different outer objects do always have separate identities.

@@ -80,6 +92,14 @@ public ClassFactory(final SSymbol name, final MixinDefinition mixinDef,

this.superclassAndMixins = superclassAndMixins;

if (VmSettings.SNAPSHOTS_ENABLED) {
CompilerDirectives.transferToInterpreter();

This comment has been minimized.

Copy link
@smarr

smarr Oct 8, 2018

Owner

This transferToInterpreter() shouldn't be done here, because it interferes with the VM.callerNeedsToBeOptimized a few lines below. Perhaps initializing serializationRoot should come afterwards. Either way, we expect this code to be executed only on the slow path.

public SSymbol getIdentifier() {
if (identifier == null) {
CompilerDirectives.transferToInterpreterAndInvalidate();
identifier = Symbols.symbolFor(mixinDef.getIdentifier());

This comment has been minimized.

Copy link
@smarr

smarr Oct 8, 2018

Owner

Hm, why is the caching logic here and not in the mixin definition?

@smarr

smarr approved these changes Oct 15, 2018

@smarr smarr merged commit 64b4961 into smarr:dev Oct 15, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive 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 15, 2018

@daumayr daumayr deleted the daumayr:Actor-Snapshots branch Oct 16, 2018

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.