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

make computationgraph constructor private to force use of factory method #47

Merged
merged 2 commits into from Feb 23, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
36 changes: 11 additions & 25 deletions swig/README.md
Expand Up @@ -149,7 +149,7 @@ it also on CPU. If you just use the Scala `myInitialize()` helper you should be

## Differences between Scala and C++

### Delete Your ComputationGraphs
### `ComputationGraph.getNew`

DyNet does not like it if you try to instantiate more than one ComputationGraph at a time.

Expand All @@ -162,27 +162,21 @@ for (int i = 0; i < NUM_TIMES; i++) {
}
```

This works because here `cg` gets destructed each time it goes out of scope. The same is not true in Scala
This works because here `cg` gets destructed each time it goes out of scope.

```scala
// BAD! DO NOT DO THIS!
for (i <- 0 until NUM_TIMES) {
val cg = new ComputationGraph
// do some computations
}
```

The underlying C++ ComputationGraph gets destructed at some point
If you were to write the analogous code in Scala
(generating a new ComputationGraph each iteration)
the underlying C++ ComputationGraph would get destructed at some point
(presumably whenever the Java GC runs),
but not immediately. As a result, your program will crash with the dreaded
but not at the end of each loop.
As a result, your program would crash with the dreaded

```
[error] Memory allocator assumes only a single ComputationGraph at a time.
```

To avoid this, we added a static `getNew` method that remembers the
last requested ComputationGraph and deletes it every time you request
another.
To prevent this, in Scala
you can only get new `ComputationGraph`s using the static `getNew` method:

```scala
for (i <- 0 until NUM_TIMES) {
Expand All @@ -191,16 +185,8 @@ for (i <- 0 until NUM_TIMES) {
}
```

Alternatively, you can manually call `.delete()` on each
`ComputationGraph` instance when you are done with it:

```scala
for (i <- 0 until NUM_TIMES) {
val cg = new ComputationGraph
// do some computations
cg.delete()
}
```
which keeps track of the previously allocated `ComputationGraph` and deletes it
whenever you request a new one.

### `std::vector`s

Expand Down
3 changes: 3 additions & 0 deletions swig/dynet_swig.i
Expand Up @@ -631,11 +631,14 @@ Expression trace_of_product(const Expression& x, const Expression& y);
if (singletonInstance != null) {
singletonInstance.delete();
}

singletonInstance = new ComputationGraph();
return singletonInstance;
}
%}

%javamethodmodifiers ComputationGraph::ComputationGraph() "private";

struct ComputationGraph {
ComputationGraph();
~ComputationGraph();
Expand Down
2 changes: 1 addition & 1 deletion swig/src/main/java/edu/cmu/dynet/examples/XorExample.java
Expand Up @@ -24,7 +24,7 @@ public static void main(String[] args) {
System.out.println("Dynet initialized!");
Model m = new Model();
SimpleSGDTrainer sgd = new SimpleSGDTrainer(m);
ComputationGraph cg = new ComputationGraph();
ComputationGraph cg = ComputationGraph.getNew();

// Declare parameters
Parameter p_W = m.add_parameters(makeDim(new int[]{HIDDEN_SIZE, 2}));
Expand Down