Skip to content

Commit

Permalink
Re-defined constants are not "in-lined" but re-evaluated on *every*
Browse files Browse the repository at this point in the history
invocation
(needs more aggressive constant propagation).

Addresses Github issue #648
#648

[Feature][TLC]
  • Loading branch information
lemmy committed Jan 8, 2022
1 parent c40c035 commit 7d936f2
Show file tree
Hide file tree
Showing 10 changed files with 340 additions and 19 deletions.
2 changes: 1 addition & 1 deletion general/performance/PaxosMadeSimple/MC.tla
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ Permutations(const_144139943710210000) \union Permutations(const_144139943711311

\* CONSTANT definitions @modelParameterConstants:2Quorum
const_144139943713313000 ==
{Q \in SUBSET Acceptor : Cardinality(Q) > Cardinality(Acceptor)\div 2}
TLCEval({Q \in SUBSET Acceptor : Cardinality(Q) > Cardinality(Acceptor)\div 2})
----

\* CONSTANT definition @modelParameterDefinitions:1
Expand Down
5 changes: 5 additions & 0 deletions general/performance/stats.R
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ data <- read.csv(header=TRUE, sep = "#", file = here("out_run-stats.csv"))
## a commit only changes auxiliary files.
## Replace git commit short-hash f91... with df1...
data[data == "f91c7b0"] <- "df144c5"
data[data == "46de326"] <- "df144c5"


data[data == "0f6a80b"] <- "1eb600d"
data[data == "0b93602"] <- "1eb600d"

## Convert epoch to date.
Expand All @@ -30,6 +34,7 @@ s <- scatterD3(data = data,
xlab = "RevTag", ylab = "log(Time&Throughput)",
col_var=Spec
)
plot(s)

library(htmlwidgets)
saveWidget(s, file="index.html")
Expand Down
25 changes: 13 additions & 12 deletions tlatools/org.lamport.tlatools/src/tlc2/module/TLC.java
Original file line number Diff line number Diff line change
Expand Up @@ -444,18 +444,19 @@ public static Value Any()
* @param val
* @return
*/
public static Value TLCEval(Value val) {
Value evalVal = val.toSetEnum();
if (evalVal != null) {
return evalVal;
}
evalVal = val.toFcnRcd();
if (evalVal != null) {
return evalVal;
}
// System.out.println("TLCEval gets no conversion");
return val;
}
// public static Value TLCEval(Value val) {
// Value evalVal = val.toSetEnum();
// if (evalVal != null) {
// return evalVal;
// }
// evalVal = val.toFcnRcd();
// if (evalVal != null) {
// return evalVal;
// }
// // System.out.println("TLCEval gets no conversion");
// return val;
// }

/*
public static Value FApply(Value f, Value op, Value base) {
FcnRcdValue fcn = f.toFcnRcd();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,6 @@ public class TLCBuiltInOverrides implements ITLCOverrides {
@SuppressWarnings("rawtypes")
@Override
public Class[] get() {
return new Class[] { TLCGetSet.class, TLCExt.class, Json.class };
return new Class[] { TLCGetSet.class, TLCEval.class, TLCExt.class, Json.class };
}
}
131 changes: 131 additions & 0 deletions tlatools/org.lamport.tlatools/src/tlc2/module/TLCEval.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
/*******************************************************************************
* Copyright (c) 2022 Microsoft Research. All rights reserved.
*
* The MIT License (MIT)
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies
* of the Software, and to permit persons to whom the Software is furnished to do
* so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in all
* copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
* FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
* COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN
* AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
* WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
*
* Contributors:
* Markus Alexander Kuppe - initial API and implementation
******************************************************************************/
package tlc2.module;

import java.util.concurrent.locks.ReadWriteLock;
import java.util.concurrent.locks.ReentrantReadWriteLock;

import tla2sany.semantic.ExprOrOpArgNode;
import tla2sany.semantic.LevelConstants;
import tlc2.overrides.Evaluation;
import tlc2.tool.TLCState;
import tlc2.tool.coverage.CostModel;
import tlc2.tool.impl.Tool;
import tlc2.tool.impl.WorkerValue;
import tlc2.util.Context;
import tlc2.value.ValueConstants;
import tlc2.value.impl.Value;

public class TLCEval implements ValueConstants {

public static final long serialVersionUID = 20220105L;

private static final ReadWriteLock lock = new ReentrantReadWriteLock();

private static Value convert(final Value eval) {
// Legacy implementation of TLCEval taken from TLC.java
Value evalVal = eval.toSetEnum();
if (evalVal != null) {
return evalVal;
}
evalVal = eval.toFcnRcd();
if (evalVal != null) {
return evalVal;
}
return eval;
}

/**
* Implements TLCEval, which causes TLC to eagerly evaluate the value. Useful
* for preventing inefficiency caused by lazy evaluation defeating efforts at
* common subexpression elimination.
*/
@Evaluation(definition = "TLCEval", module = "TLC", warn = false, silent = true)
public static Value tlcEval(final Tool tool, final ExprOrOpArgNode[] args, final Context c, final TLCState s0,
final TLCState s1, final int control, final CostModel cm) {
// TLCEval has a single parameter:
final ExprOrOpArgNode arg = args[0];

if (arg.getLevel() > LevelConstants.ConstantLevel) {
// For a non-constant expression, all we can do is to evaluate
// and convert the value according to the old implementation
// of TLCEval.
// Since there is no sharing going on, there is no need to deal
// with WorkerValue here.

// The value that a constant-level expression evaluates to is stored in the
// semantic graph.
// For a state-level formula, the value could be kept in a transient member
// of the state. This effort doesn't seem worth it, though.
return convert(tool.eval(arg, c, s0, s1, control, cm));
} else if (!c.isEmpty()) {
// If a constant expression has a context, e.g. a parameter, we
// cannot cache the value.
return convert(tool.eval(arg, c, s0, s1, control, cm));
}

return tlcEvalConst(tool, arg, cm);
}

private static Value tlcEvalConst(Tool tool, ExprOrOpArgNode arg, CostModel cm) {
assert arg.getLevel() == LevelConstants.ConstantLevel;

lock.readLock().lock();

// Read with ReadLock
Object obj = WorkerValue.mux(arg.getToolObject(tool.getId()));
if (obj != null) {
// Return the cached value.
try {
return (Value) obj;
} finally {
lock.readLock().unlock();
}
}

// Slow-path below. Note that ReentrantRWLock deadlocks when
// upgrading a read to a write-lock, but we don't need this here.
lock.readLock().unlock();

lock.writeLock().lock();
try {
// Re-read with WriteLock in case another thread obtained
// the write lock while this thread waited.
obj = WorkerValue.mux(arg.getToolObject(tool.getId()));
if (obj != null) {
return (Value) obj;
}

// Create/Write the value!
Value eval = (Value) WorkerValue.demux(tool, arg, cm);
eval = convert(eval);
arg.setToolObject(tool.getId(), eval);
return eval;
} finally {
lock.writeLock().unlock();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -247,8 +247,42 @@ else if (val != null && val instanceof OpDefNode) {
if (opDef.getArity() == 0) {
try {
Object defVal = WorkerValue.demux(opDefEvaluator, opDef.getBody());
opDef.setToolObject(toolId, defVal);
constantDefns.computeIfAbsent(mod, key -> new HashMap<OpDefOrDeclNode, Object>()).put(opDef, defVal);
opDef.setToolObject(toolId, defVal);
// https://github.com/tlaplus/tlaplus/issues/648
//
// Without consts[i].setTool... below, TLC re-evaluates CONSTANT definitions on
// every invocation. With consts[i].setTool..., TLC caches values. Here is the
// reason why consts[i].setTool... is commented:
//
// A) For "cheap" definitions such as CONSTANT N = 4, caching values makes no
// difference.
//
// B) For more expensive expressions, such as PaxosMadeSimple.tla the performance
// gain is around 10%.
//
// Quorum == {i \in SUBSET(Server) : Cardinality(i) * 2 > Cardinality(Server)}
//
// CONSTANT Q <- Quorum
//
// C) However, half of our user base expects the following spec to probabilistically
// have 2 to 4 distinct states:
//
// EXTENDS TLC
// R == RandomElement({1,2,3})
// CONSTANT C
// VARIABLE x
// Spec == x = 0 /\ [][x' = C]_x
//
// CONSTANT C <- R
//
// Therefore, we let the user decide by giving her TLC!TLCEval to wrap expressive
// constant definitions when necessary:
//
// R == TLCEval(RandomElement({1,2,3}))
//
// consts[i].setToolObject(toolId, defVal);

constantDefns.computeIfAbsent(mod, key -> new HashMap<OpDefOrDeclNode, Object>()).put(opDef, defVal);
} catch (Assert.TLCRuntimeException | EvalException e) {
final String addendum = (e instanceof EvalException) ? "" : (" - specifically: " + e.getMessage());
Assert.fail(EC.TLC_CONFIG_SUBSTITUTION_NON_CONSTANT,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,12 +103,18 @@ Under these two assumptions, both Workers will race to enumerate (fingerprint) `
2) [Value#deepCopy](https://github.com/tlaplus/tlaplus/blob/63bab42e79e750b7ac033cfe7196d8f0b53e861e/tlatools/src/tlc2/value/IValue.java#L104) - required to create the copies of V<sub>SG</sub> - is [broken for most value types](https://github.com/tlaplus/tlaplus/blob/63bab42e79e750b7ac033cfe7196d8f0b53e861e/tlatools/src/tlc2/value/impl/SubsetValue.java#L232) (can probably be fixed).
*/
public class WorkerValue {

/*
* Demuxing is supposed to be called only once per sn/opDef whereas muxing is called many many times.
*/
public static Object demux(final OpDefEvaluator spec, final ExprOrOpArgNode en) {
final IValue defVal = spec.eval(en, Context.Empty, TLCState.Empty, CostModel.DO_NOT_RECORD);
return demux(spec, en, CostModel.DO_NOT_RECORD);
}

/*
* Demuxing is supposed to be called only once per sn/opDef whereas muxing is called many many times.
*/
public static Object demux(final OpDefEvaluator spec, final ExprOrOpArgNode en, final CostModel cm) {
final IValue defVal = spec.eval(en, Context.Empty, TLCState.Empty, cm);
defVal.deepNormalize();

if (defVal.mutates() && TLCGlobals.getNumWorkers() > 1) {
Expand All @@ -118,7 +124,7 @@ public static Object demux(final OpDefEvaluator spec, final ExprOrOpArgNode en)
for (int i = 1; i < values.length; i++) {
// Ideally, we could invoke IValue#deepCopy here instead of evaluating opDef again. However,
// IValue#deepCopy doesn't create copies for most values.
values[i] = spec.eval(en, Context.Empty, TLCState.Empty, CostModel.DO_NOT_RECORD);
values[i] = spec.eval(en, Context.Empty, TLCState.Empty, cm);
values[i].deepNormalize();
}

Expand Down
4 changes: 4 additions & 0 deletions tlatools/org.lamport.tlatools/test-model/Github648.cfg
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
INIT Init
NEXT Next
INVARIANT Inv
CONSTANT Graph <- TestGraph
51 changes: 51 additions & 0 deletions tlatools/org.lamport.tlatools/test-model/Github648.tla
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
--------------------- MODULE Github648 -----------------------------
EXTENDS TLC, Naturals, Sequences, FiniteSets

BoundedSeqTLCEval(S, n) ==
\* The TLCEval here must not cache the value, because it depends on the
\* context, ie. S and n.
UNION {TLCEval([1..m -> S]) : m \in 0..n}

ASSUME Cardinality(BoundedSeqTLCEval(BoundedSeqTLCEval({1,2}, 2), 2)) = 57

-----------------------------------------------------------------------------
BoundedSeq(S, n) ==
UNION {[1..m -> S] : m \in 0..n}

-----------------------------------------------------------------------------
DirectedGraphs(nodes) ==
[edge : SUBSET (nodes \X nodes)]

TestGraph ==
\* The definition of TestGraph is evaluated multiple times. If the def.
\* involves TLC!RandomElement or Randomization!* the invariant Inv below
\* will be violated *without* the TLCEval.
TLCEval(
LET g == RandomElement(DirectedGraphs(1..3))
IN [ edge |-> g.edge \cup {<<1,1>>} ]
)

-----------------------------------------------------------------------------
CONSTANT Graph

VARIABLE v, w
vars == <<v, w>>

Init ==
/\ v \in Graph.edge
/\ w \in Graph.edge

Next ==
/\ v' \in Graph.edge
/\ w' \in Graph.edge

Inv ==
/\ v \in Graph.edge
/\ v \in TestGraph.edge
/\ w \in Graph.edge
/\ w \in TestGraph.edge
/\ TLCEval(Cardinality(
TLCEval(BoundedSeq(
TLCEval(BoundedSeq({1,2,3}, 3)), 3)))) = 65641

=============================================================================
Loading

3 comments on commit 7d936f2

@lemmy
Copy link
Member Author

@lemmy lemmy commented on 7d936f2 Jan 8, 2022

Choose a reason for hiding this comment

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

@Calvin-L
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks fine to me.

RandomElement seems like a very tricky feature to support in a language where expressions are mathematical formulas and not programs... what's here seems like a reasonable enough approach, given that TLC already supports RandomElement.

@lemmy
Copy link
Member Author

@lemmy lemmy commented on 7d936f2 Apr 13, 2022

Choose a reason for hiding this comment

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

Archiving the specs here that were used as part of this work:

---- MODULE ConstIsConst -----
EXTENDS Naturals,TLC

CONSTANT C

VARIABLE x

Expr ==
    IF x=x THEN Print("Evaluated", 1 + 1) ELSE 42

Spec == x = C /\ [][x' = C]_x

=====
---- CONFIG ConstIsConst -----
SPECIFICATION Spec
CONSTANT C <- Expr
==========================
--------------------- MODULE ConstantProcessing -----------------------------
EXTENDS TLC, Naturals, SequencesExt, TLCExt

SomeSet ==
    \* LET SomeSubset(n) == SUBSET (n \X n) IN 
    \* The definition of TestSet is evaluated multiple times.  If the def.
    \* involves TLC!RandomElement or Randomization!* the invariant Inv below
    \* will be violated.
    SUBSET {1,2,3,4,5,6,7,8,9,10}
    \* Print(TRUE, SUBSET {1,2,3,4,5,6,7,8,9,10})\*RandomElement(SUBSET ((1..3) \X (1..3)))
    \* CHOOSE g \in SomeSubset(1..3): TRUE

-----------------------------------------------------------------------------

CONSTANT S

VARIABLE v, w
vars == <<v, w>>

Init ==
    /\ v \in S
    /\ w \in S

Next ==
    UNCHANGED vars

Inv ==
    /\ v \in S
    /\ w \in S

=============================================================================
---- CONFIG ConstantProcessing ----------------------------------------------
INIT Init
NEXT Next
\* INVARIANT Inv
CONSTANT S <- SomeSet
=============================================================================

Please sign in to comment.