Change in bazel[master]: Don't serialize events in Skyframe.

4 views
Skip to first unread message

Janak Ramakrishnan (Gerrit)

unread,
Jul 13, 2015, 11:47:18 AM7/13/15
to bazel-de...@googlegroups.com
Janak Ramakrishnan has uploaded a new change for review.

https://bazel-review.googlesource.com/1612

Change subject: Don't serialize events in Skyframe.
......................................................................

Don't serialize events in Skyframe.

It's unlikely that users will want to see errors that were emitted on
a previous build and stored, and we're not good at serializing nested
sets anyway.

Change-Id: Ib09758b4169ade5ec10a32bd6a40c23151069c41
---
M src/main/java/com/google/devtools/build/skyframe/ValueWithMetadata.java
1 file changed, 28 insertions(+), 5 deletions(-)



diff --git
a/src/main/java/com/google/devtools/build/skyframe/ValueWithMetadata.java
b/src/main/java/com/google/devtools/build/skyframe/ValueWithMetadata.java
index 3795832..bbe857b 100644
---
a/src/main/java/com/google/devtools/build/skyframe/ValueWithMetadata.java
+++
b/src/main/java/com/google/devtools/build/skyframe/ValueWithMetadata.java
@@ -17,6 +17,9 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.collect.nestedset.Order;

+import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
import java.util.Objects;

import javax.annotation.Nullable;
@@ -75,8 +78,8 @@

/** Implementation of {@link ValueWithMetadata} for the value case. */
public static final class ValueWithEvents extends ValueWithMetadata {
-
- private final NestedSet<TaggedEvents> transitiveEvents;
+ // Non-final only for serialization.
+ private NestedSet<TaggedEvents> transitiveEvents;

public ValueWithEvents(SkyValue value, NestedSet<TaggedEvents>
transitiveEvents) {
super(Preconditions.checkNotNull(value));
@@ -121,13 +124,22 @@

@Override
public String toString() { return value.toString(); }
+
+ private void readObject(ObjectInputStream stream)
+ throws IOException, ClassNotFoundException {
+ transitiveEvents = NO_EVENTS;
+ }
+
+ private void writeObject(ObjectOutputStream stream) throws IOException
{
+ // Do nothing -- transitive events shouldn't be serialized.
+ }
}

/** Implementation of {@link ValueWithMetadata} for the error case. */
public static final class ErrorInfoValue extends ValueWithMetadata {
-
- private final ErrorInfo errorInfo;
- private final NestedSet<TaggedEvents> transitiveEvents;
+ // Non-final only for serialization.
+ private ErrorInfo errorInfo;
+ private NestedSet<TaggedEvents> transitiveEvents;

public ErrorInfoValue(ErrorInfo errorInfo, @Nullable SkyValue value,
NestedSet<TaggedEvents> transitiveEvents) {
@@ -184,6 +196,17 @@
}
return result.toString();
}
+
+ private void readObject(ObjectInputStream stream)
+ throws IOException, ClassNotFoundException {
+ errorInfo = (ErrorInfo) stream.readObject();
+ transitiveEvents = NO_EVENTS;
+ }
+
+ private void writeObject(ObjectOutputStream stream) throws IOException
{
+ stream.writeObject(errorInfo);
+ // Don't serialize transitive events.
+ }
}

public static SkyValue justValue(SkyValue value) {

--
To view, visit https://bazel-review.googlesource.com/1612
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib09758b4169ade5ec10a32bd6a40c23151069c41
Gerrit-PatchSet: 1
Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Janak Ramakrishnan <jan...@google.com>

Eric Fellheimer (Gerrit)

unread,
Jul 13, 2015, 1:00:07 PM7/13/15
to Janak Ramakrishnan
Eric Fellheimer has posted comments on this change.

Change subject: Don't serialize events in Skyframe.
......................................................................


Patch Set 1: Code-Review+2

--
To view, visit https://bazel-review.googlesource.com/1612
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib09758b4169ade5ec10a32bd6a40c23151069c41
Gerrit-PatchSet: 1
Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Janak Ramakrishnan <jan...@google.com>
Gerrit-Reviewer: Eric Fellheimer <fe...@google.com>
Gerrit-HasComments: No

Janak Ramakrishnan (Gerrit)

unread,
Jul 13, 2015, 6:49:32 PM7/13/15
to Eric Fellheimer
Hello Eric Fellheimer,

I'd like you to reexamine a change. Please visit

https://bazel-review.googlesource.com/1612

to look at the new patch set (#2).

Change subject: Don't serialize events in Skyframe.
......................................................................

Don't serialize events in Skyframe.

It's unlikely that users will want to see errors that were emitted on
a previous build and stored, and we're not good at serializing nested
sets anyway.

This change introduces the extremely undesirable property that equal
objects may have unequal hash codes.

Change-Id: Ib09758b4169ade5ec10a32bd6a40c23151069c41
---
M src/main/java/com/google/devtools/build/skyframe/ValueWithMetadata.java
1 file changed, 36 insertions(+), 22 deletions(-)


--
To view, visit https://bazel-review.googlesource.com/1612
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib09758b4169ade5ec10a32bd6a40c23151069c41
Gerrit-PatchSet: 2
Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Janak Ramakrishnan <jan...@google.com>
Gerrit-Reviewer: Eric Fellheimer <fe...@google.com>

Janak Ramakrishnan (Gerrit)

unread,
Jul 13, 2015, 6:51:13 PM7/13/15
to Eric Fellheimer
Janak Ramakrishnan has posted comments on this change.

Change subject: Don't serialize events in Skyframe.
......................................................................


Patch Set 1:

PTAL. I realized the previous change was wrong because if there were
emitted events, we would lose change pruning for those nodes (although
before my previous cl we didn't have it anyway). Given the invariant this
now violates, I'm debating serializing the shallow hash code and using that
when checking for equality. Thoughts?

--
To view, visit https://bazel-review.googlesource.com/1612
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib09758b4169ade5ec10a32bd6a40c23151069c41
Gerrit-PatchSet: 1
Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Janak Ramakrishnan <jan...@google.com>
Gerrit-Reviewer: Eric Fellheimer <fe...@google.com>
Gerrit-Reviewer: Janak Ramakrishnan <jan...@google.com>
Gerrit-HasComments: No

Nathan Harmata (Gerrit)

unread,
Jul 13, 2015, 6:58:57 PM7/13/15
to Janak Ramakrishnan, Eric Fellheimer
Nathan Harmata has posted comments on this change.

Change subject: Don't serialize events in Skyframe.
......................................................................


Patch Set 2:

i think that'd potentially violate
https://github.com/google/bazel/blob/5fe79331f9160ae8b37f907ddbaa5f080dfc8185/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSet.java#L106
because of
https://github.com/google/bazel/blob/5fe79331f9160ae8b37f907ddbaa5f080dfc8185/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSet.java#L96.
have you audited all the implementations?

--
To view, visit https://bazel-review.googlesource.com/1612
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib09758b4169ade5ec10a32bd6a40c23151069c41
Gerrit-PatchSet: 2
Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Janak Ramakrishnan <jan...@google.com>
Gerrit-Reviewer: Eric Fellheimer <fe...@google.com>
Gerrit-Reviewer: Janak Ramakrishnan <jan...@google.com>
Gerrit-Reviewer: Nathan Harmata <nhar...@google.com>
Gerrit-HasComments: No

Janak Ramakrishnan (Gerrit)

unread,
Jul 13, 2015, 7:09:29 PM7/13/15
to Nathan Harmata, Eric Fellheimer
Janak Ramakrishnan has posted comments on this change.

Change subject: Don't serialize events in Skyframe.
......................................................................


Patch Set 2:

I don't see how changing hash code for a class that embeds nested sets can
violate anything about nested sets. Can you say more?

--
To view, visit https://bazel-review.googlesource.com/1612
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib09758b4169ade5ec10a32bd6a40c23151069c41
Gerrit-PatchSet: 2
Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Janak Ramakrishnan <jan...@google.com>

Nathan Harmata (Gerrit)

unread,
Jul 13, 2015, 7:29:35 PM7/13/15
to Janak Ramakrishnan, Eric Fellheimer
Nathan Harmata has posted comments on this change.

Change subject: Don't serialize events in Skyframe.
......................................................................


Patch Set 2:

(4 comments)

https://bazel-review.googlesource.com/#/c/1612/2/src/main/java/com/google/devtools/build/skyframe/ValueWithMetadata.java
File
src/main/java/com/google/devtools/build/skyframe/ValueWithMetadata.java:

PS2, Line 79: NestedSet
here and below: mark the parameters as @Nullable


PS2, Line 85: equals
shallow equals is sound, but doing the 'equalIfOneNull' stuff isn't sound
(for arbitrary callers of ValueWithMetadata#equals). i think you need to
provide more context in this comment and/or implement a different method
than #equals in order to motivate this decision. e.g. suppose we have

ValueWithMetaData vm1 = ...
ValueWithMetadata vm2 = ...
ValueWithMetadata vm2_d = ... // a deserialized instance of the
serialization of vm2

i think it's both undesirable* and confusing that

'vm1.equals(vm2)' can be false yet 'vm1.equals(vm2_d)' can be true

*undesirable unless the reader has the missing context i'm referring to.


PS2, Line 95: null
Add a comment explaining what the semantics of a null value are here


PS2, Line 96: transient
@Nullable


--
To view, visit https://bazel-review.googlesource.com/1612
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib09758b4169ade5ec10a32bd6a40c23151069c41
Gerrit-PatchSet: 2
Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Janak Ramakrishnan <jan...@google.com>
Gerrit-Reviewer: Eric Fellheimer <fe...@google.com>
Gerrit-Reviewer: Janak Ramakrishnan <jan...@google.com>
Gerrit-Reviewer: Nathan Harmata <nhar...@google.com>
Gerrit-HasComments: Yes

Nathan Harmata (Gerrit)

unread,
Jul 13, 2015, 7:30:46 PM7/13/15
to Janak Ramakrishnan, Eric Fellheimer
Nathan Harmata has posted comments on this change.

Change subject: Don't serialize events in Skyframe.
......................................................................


Patch Set 2:

> I don't see how changing hash code for a class that embeds nested sets
> can violate anything about nested sets.

you're right; i didn't fully understand your earlier comment and i was just
being dumb anyway

--
To view, visit https://bazel-review.googlesource.com/1612
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib09758b4169ade5ec10a32bd6a40c23151069c41
Gerrit-PatchSet: 2
Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Janak Ramakrishnan <jan...@google.com>
Gerrit-Reviewer: Eric Fellheimer <fe...@google.com>
Gerrit-Reviewer: Janak Ramakrishnan <jan...@google.com>
Gerrit-Reviewer: Nathan Harmata <nhar...@google.com>
Gerrit-HasComments: No

Janak Ramakrishnan (Gerrit)

unread,
Jul 15, 2015, 6:08:43 AM7/15/15
to Nathan Harmata, Eric Fellheimer
Janak Ramakrishnan has posted comments on this change.

Change subject: Don't serialize events in Skyframe.
......................................................................


Patch Set 2:

Abandoning this change due to the violation that would ensue of
transitivity of equality.

--
To view, visit https://bazel-review.googlesource.com/1612
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib09758b4169ade5ec10a32bd6a40c23151069c41
Gerrit-PatchSet: 2
Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Janak Ramakrishnan <jan...@google.com>

Janak Ramakrishnan (Gerrit)

unread,
Jul 15, 2015, 6:08:51 AM7/15/15
to Nathan Harmata, Eric Fellheimer
Janak Ramakrishnan has abandoned this change.

Change subject: Don't serialize events in Skyframe.
......................................................................


Abandoned

--
To view, visit https://bazel-review.googlesource.com/1612
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: Ib09758b4169ade5ec10a32bd6a40c23151069c41
Gerrit-PatchSet: 2
Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Janak Ramakrishnan <jan...@google.com>
Reply all
Reply to author
Forward
0 new messages