Skip to content

static-contracts: improve optimizer's test for flat scs #631

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

Merged
merged 1 commit into from
Nov 7, 2017

Conversation

bennn
Copy link
Contributor

@bennn bennn commented Oct 14, 2017

This is one idea for fixing #628 .

I'm not happy about re-computing recursive-kinds in the optimizer,
but I don't see how to tell whether a name/sc is flat without it,
and I didn't see an easy way to share the optimizer's recursive-kinds with instantiate.rkt


Improve the static contract optimizer's ability to recognize flat contracts:

  1. get the kind of all recursive, named contracts
  2. any name/sc with 'flat kind is a flat contract

Step 1 is repeated later in the instantiate pass.

@pnwamk
Copy link
Member

pnwamk commented Oct 14, 2017

Two questions/comments off the top of my head:

  1. How does this affect compile time performance? Generally, and in code that spends a lot of time in contract generation (e.g. https://github.com/akuhlens/schml, and the schml examples in https://github.com/pnwamk/tr-performance)

  2. More generally, I think we may just need the structs in our static-contract IRL to cache what kind of contract they are (i.e. flat, chaperone, impersonator) so we only compute this once not only for recursive name/sc's, but just in general for all static contracts. But perhaps this is a larger beast to tackle later (really we should just make a nice, clean DSL for defining static-contract structs like we have for Rep's)

@bennn
Copy link
Contributor Author

bennn commented Oct 16, 2017

  1. Too much slower. Here's the current master branch vs. this branch:
acquire: 0.94 (i.e. slower, 18.19s (σ 0.42) to 19.41s (σ 0.21))
dungeon: 0.97 (i.e. slower, 10.08s (σ 0.08) to 10.36s (σ 0.59))
forth: 1.0 (i.e. faster, 5.6s (σ 0.05) to 5.6s (σ 0.05))
fsm: 1.0 (i.e. slower, 5.65s (σ 0.02) to 5.67s (σ 0.05))
math-flonum: 0.99 (i.e. slower, 4.53s (σ 0.02) to 4.56s (σ 0.04))
new-metrics: 1.06 (i.e. faster, 3.79s (σ 0.1) to 3.58s (σ 0.16))
old-metrics: 1.0 (i.e. faster, 2.87s (σ 0.04) to 2.86s (σ 0.02))
parser: 1.0 (i.e. faster, 2.43s (σ 0.02) to 2.42s (σ 0.01))
schml-interp-casts-help: 0.69 (i.e. slower, 23.87s (σ 0.08) to 34.75s (σ 0.07))
schml-specify-rep: 0.94 (i.e. slower, 9.93s (σ 0.03) to 10.59s (σ 0.47))

@samth
Copy link
Member

samth commented Oct 16, 2017

What if you just try a simple solution and memoize the computation of recursive-kinds with a hash table?

@bennn
Copy link
Contributor Author

bennn commented Oct 17, 2017

I'm not sure how to memoize.
At first I tried making a function from sc to recursive-kinds that used the sc as an eq? cache key, but that's a bad key because the optimizer changes the sc.

The commit I just pushed here adds a new function instantiate/optimize that computes recursive-kinds once. It's about 4s slower than master. Looking into improving it.

EDIT: 1/3 travis checks failed because I didn't update the contract for optimize

@bennn
Copy link
Contributor Author

bennn commented Oct 17, 2017

The latest commit has better performance,

benchmark master e1a2a77
acquire 18.19s (σ 0.42) 20.35s (σ 0.66)
dungeon 10.08s (σ 0.08) 19.66s (σ 17.65)
forth 5.6s (σ 0.05) 5.96s (σ 0.13)
fsm 5.65s (σ 0.02) 6.06s (σ 0.13)
math-flonum 4.53s (σ 0.02) 5.03s (σ 0.25)
new-metrics 3.79s (σ 0.1) 4.02s (σ 0.2)
old-metrics 2.87s (σ 0.04) 2.95s (σ 0.04)
parser 2.43s (σ 0.02) 2.46s (σ 0.04)
schml-interp-casts-help 23.87s (σ 0.08) 25.77s (σ 0.48)
schml-specify-rep 9.93s (σ 0.03) 10.61s (σ 0.18)

EDIT: failed a test

Contract Tests > Unit Tests > Typed Racket Tests > Any-Syntax for (syntax (syntax A)) in syntax?
FAILURE
type:        Syntax
test-value:  (syntax (syntax A))
required a flat contract but generated a chaperone contract

@samth
Copy link
Member

samth commented Oct 17, 2017

What if we have a mutable field on all scs, which we update with the kind when we learn it, and transfer when we optimize? That should fix the problem of keys changing.

@bennn
Copy link
Contributor Author

bennn commented Oct 18, 2017

I tried a simple version of that for name/sc here: fc15feb
but it seemed slow after 1 measurement so I moved on to the free-id-set version in this commit.

I'll try it again.

@bennn
Copy link
Contributor Author

bennn commented Oct 19, 2017

This PR currently: (1) solves for the kinds of recursive contracts before optimizing and (2) shares the result with optimize and instantiate. Performance seem ok (see below). I think this is the right fix.

I thought about adding a mutable "kind" field to name/sc and recursive-sc, but two problems stopped me:

  1. (hard) The constraint solver works with identifiers, not scs. I didn't see an easy way to jump from an identifier back to its parent name/sc or recursive-sc, and didn't want to traverse the whole sc again to update those fields with data from a recursive-kinds hashtable.
  2. (more refactoring) If scs know their kind, then instantiate probably doesn't need the table of recursive kinds at all, and should be refactored

Here's a table with compile times from the tr-performance repo.

  • master is TR commit 26f65f1

  • e1a2a77 adds extra state to the implementation of name/sc

  • f59df64 has optimize and instantiate share the table of recursive-kinds

  • e7be251 is like f59df64, but with those make-XXX functions in optimize.rkt to save a little performance

    benchmark master e1a2a77 f59df64 e7be251
    acquire 18.19s (σ 0.42) 20.35s (σ 0.66) 19.91s (σ 1.02) 19.86s (σ 1.37)
    dungeon 10.08s (σ 0.08) 19.66s (σ 17.65) 25.67s (σ 20.22) 21.51s (σ 18.01)
    forth 5.6s (σ 0.05) 5.96s (σ 0.13) 5.81s (σ 0.03) 5.75s (σ 0.04)
    fsm 5.65s (σ 0.02) 6.06s (σ 0.13) 6.05s (σ 0.07) 5.98s (σ 0.02)
    math-flonum 4.53s (σ 0.02) 5.03s (σ 0.25) 6.44s (σ 0.99) 4.85s (σ 0.02)
    new-metrics 3.79s (σ 0.1) 4.02s (σ 0.2) 4.79s (σ 0.64) 4.04s (σ 0.1)
    old-metrics 2.87s (σ 0.04) 2.95s (σ 0.04) 3.28s (σ 0.21) 3.06s (σ 0.02)
    parser 2.43s (σ 0.02) 2.46s (σ 0.04) 2.8s (σ 0.21) 2.62s (σ 0.03)
    schml-interp-casts-help 23.87s (σ 0.08) 25.77s (σ 0.48) 26.0s (σ 0.12)) 24.41s (σ 0.05)
    schml-specify-rep 9.93s (σ 0.03) 10.61s (σ 0.18) 11.16s (σ 0.42) 10.64s (σ 0.09)

@samth
Copy link
Member

samth commented Oct 19, 2017

What is going on with dungeon?

Also, can you run this with the tr-timing logging, and see where the extra time is? In particular, is the extra time actually in this solving, or in the expansion of the (now-correct) contracts?

@pnwamk
Copy link
Member

pnwamk commented Oct 19, 2017

Speculation: the Schml compiler hits awful non-deterministic performance in our contract generation -- I believe caused by the iteration order of free-id-tables/sets.

This could be why the σ is so high for dungeon.

If so, I think we should experiment with (1) using a deterministic dictionary for free ids, or (2) see if the cost of sorting the free ids is prohibitive (I worry it will be, but we should experiment and see).

@samth
Copy link
Member

samth commented Oct 19, 2017

Dungeon isn't related to Schml, though, is it?

@pnwamk
Copy link
Member

pnwamk commented Oct 19, 2017

I phrased it poorly -- I wonder if the same issue Schml is hitting in contract generation, Dungeon is also hitting. With the σ on the measurements that high, that was just my first thought of another program with highly variable contract generation times under TR.

@pnwamk
Copy link
Member

pnwamk commented Oct 19, 2017

meta comment -- I haven't examined in great detail the most recent changes, so it may not even totally apply, and I'm not suggesting Ben should go try this now: I wonder if we just refactored our static-contract representation to keep around more relevant/useful data if this all wouldn't just be simpler to do correctly (i.e. at a static-contract's definition site, we specify how its kind is calculated and cached, etc (similar to our DSL for Reps (see rep/rep-utils.rkt))

@samth
Copy link
Member

samth commented Oct 19, 2017

That might be helpful as well with #633, where the issue I'm running into is wanting to go from an sc to a type so that I know what type a contract came from.

@bennn
Copy link
Contributor Author

bennn commented Oct 19, 2017

@pnwamk I agree the sc rep should be reorganized, but I don't want to do that here.
Can you open a new issue to talk about a wishlist/spec for the new rep?

(My wishlist: (1) easier to get kind of recursive scs (2) get original type (3) check whether the sc corresponds to a "real" type constructor)

@bennn
Copy link
Contributor Author

bennn commented Oct 19, 2017

Part of the reason I don't want to do the refactoring here: IMO this PR is an "emergency fix" because programs that used to run now error (after 9df037b).

[[ I was really hoping to fix #628 with this before the release (just like #625) ]]

@bennn
Copy link
Contributor Author

bennn commented Oct 19, 2017

Re-ran dungeon twice more, comparing to 6.10.1. One run on this PR was very slow, otherwise it looks fine:

HEAD/raco make dungeon:
  times (67.62 14.01 12.51 10.88 11.77 11.48 12.27 11.92 11.37 10.88 10.98 11.04 11.08 11.3 10.76 10.7 10.96 10.99 10.9 10.85)
  mean 14.21
  stddev 12.28

6.10/raco make dungeon:
  times (11.52 9.91 10.01 9.67 10.18 9.53 10.02 9.8 9.99 9.46 9.9 10.23 9.57 10.22 9.55 10.32 9.64 10.47 9.78 10.19)
  mean 10.0
  stddev 0.45

HEAD/raco make dungeon:
  times (12.35 11.68 11.67 11.09 11.19 10.65 10.95 10.98 10.89 11.01)
  mean 11.25
  stddev 0.48

6.10/raco make dungeon:
  times (11.33 11.05 11.09 10.91 10.72 10.72 10.73 10.3 9.62 10.13)
  mean 10.66
  stddev 0.49

EDIT: nevermind these logs. We really need tr-timing for the 60-second run, but I don't have one yet.
Attached: tr-timing info for 5 runs of dungeon. I haven't read these yet.
dungeon-tr-timing.tar.gz

@samth
Copy link
Member

samth commented Oct 19, 2017

I'm still worried about that outlier. What is going on there?

Also, more broadly if we can't avoid real performance hit, do we want to revert the or/c fix until after the release?

@bennn
Copy link
Contributor Author

bennn commented Oct 19, 2017

I tried recompiling dungeon with tr-logging and wasn't able to reproduce the 60-second outlier. Specifically, I ran this command for an hour, and every test was in the 9-11 second range.

I=1; while [[ ${I} ]]; do rm -r compiled; time PLTSTDERR="error debug@tr-timing" raco make -v main.rkt >&LOG-${I}; I=$(( ${I} + 1)); done

I don't think PR adds a serious performance hit, and I don't want to revert the or/c fix because that closes a soundness bug.

(The original proposal here was slow because it was solving for recursive kinds twice. But that's not happening anymore.)

@bennn
Copy link
Contributor Author

bennn commented Oct 19, 2017

I'll try again later to reproduce the outlier. Maybe if I change the tr-performance script to use tr-logging, it'll pick up the outliner.

@samth
Copy link
Member

samth commented Oct 19, 2017

Does the logging tell you where the performance change is?

@bennn
Copy link
Contributor Author

bennn commented Oct 19, 2017

Oh right ... I will get back to you about that.

I only made logs for dungeon on HEAD, and since none of those logs were for a 60-second compile time, I didn't read them carefully.

@bennn
Copy link
Contributor Author

bennn commented Oct 20, 2017

I compared logs for acquire dungeon old-metrics new-metrics and schml-interp-casts-help. Full results attached. Some observations:

  • Everything but schml spent very little time between "Typechecking Done" and "Generated contracts" --- 70ms or less, and the difference between 6.10.1 and this PR was at most 30ms slower
  • schml spent about 10 seconds generating contracts, 13212ms on 6.10.1 and 11620ms on this PR. So this PR ran faster in that one observation.

Looks like time spent making contracts is about the same on my laptop. Now to fix the failing unit test(s).

logs.tar.gz

@samth
Copy link
Member

samth commented Oct 20, 2017

Ok, so what does take more time then?

@bennn
Copy link
Contributor Author

bennn commented Oct 20, 2017

Looks like initialization is what's slower. Everything from "Starting" to "Starting checker" is pretty consistently a few hundred milliseconds slower.

I don't think this is anything to worry about, but if you have suggestions for how I could change this PR then I can try to implement them.

Attached: filtered tr-timing logs. For each program I filtered the logs for timing events that I thought were important. First column is the event name, 2nd is the time reported on 6.10.1, 3rd is the time on this PR, 4th is the difference between the 2nd and 3rd columns.

simplified-logs.txt


I ran dungeon a few more times with the timing script. The results don't show the outlier, so I think it was a measurement error (weirdness with my laptop, or with the script, or with /usr/bin/time)

$ racket main.rkt -v -t dungeon -i 20 v6.10.1-bin pr631-bin
building dungeon....................
v6.10.1-bin/raco make dungeon:
times (12.25 11.77 11.53 11.59 11.68 11.73 10.67 10.7 10.78 10.63 10.91 10.84 11.37 10.98 10.97 11.08 10.82 11.06 10.94 11.02)
mean 11.17
stddev 0.44
done with dungeon
building dungeon....................
pr-631-bin/raco make dungeon:
times (12.71 12.05 12.61 12.63 12.47 12.37 12.37 12.19 12.42 11.64 11.56 11.43 11.65 11.66 11.49 11.56 11.48 11.24 11.41 11.42)
mean 11.92
stddev 0.49
done with dungeon

Compile time ratio (old time / new time):
dungeon: 0.94 (i.e. slower, 11.17s (σ 0.44) to 11.92s (σ 0.49))
$ racket main.rkt -v -t dungeon -i 20 v6.10.1-bin pr631-bin
building dungeon....................
v6.10.1-bin/raco make dungeon:
times (11.49 10.83 10.9 11.02 10.93 10.92 10.82 10.78 10.84 10.68 10.18 10.0 10.12 9.96 10.28 9.81 9.91 10.25 9.84 10.33)
mean 10.49
stddev 0.47
done with dungeon
building dungeon....................
pr-631-bin/raco make dungeon:
times (11.96 11.75 11.1 11.04 11.05 10.96 10.84 10.78 10.95 11.0 10.85 11.89 12.29 12.73 11.93 11.66 11.75 11.69 11.55 11.79)
mean 11.48
stddev 0.54
done with dungeon

Compile time ratio (old time / new time):
dungeon: 0.91 (i.e. slower, 10.49s (σ 0.47) to 11.48s (σ 0.54))
$ racket main.rkt -v -t dungeon -i 20 v6.10.1-bin pr631-bin
building dungeon....................
v6.10.1-bin/raco make dungeon:
times (12.82 12.74 11.39 11.33 11.34 10.87 11.54 12.14 12.54 11.17 11.05 10.79 10.8 10.8 10.84 12.03 12.15 11.35 11.82 12.42)
mean 11.6
stddev 0.67
done with dungeon
building dungeon....................
pr-631-bin/raco make dungeon:
times (12.59 12.42 11.86 11.47 11.38 11.42 11.74 11.55 11.49 11.42 13.28 12.44 12.5 11.97 13.0 12.43 12.25 12.22 11.47 10.56)
mean 11.97
stddev 0.64
done with dungeon

Compile time ratio (old time / new time):
dungeon: 0.97 (i.e. slower, 11.6s (σ 0.67) to 11.97s (σ 0.64))

@samth
Copy link
Member

samth commented Oct 20, 2017

There's no way these changes impacted initialization time. My guess is therefore that something else since 6.10 changed things. Can you test HEAD with and without this PR?

@samth
Copy link
Member

samth commented Oct 20, 2017

Also, we need to recognize that we're very late for the release. I'll talk to @racket/release about this, but we will need to decide whether we revert the or/sc changes if this is too much too late.

@bennn
Copy link
Contributor Author

bennn commented Oct 20, 2017

I'm currently installing Racket on another machine to compare 6.10.1 with TR HEAD with TR on this pull request.

Just so we're on the same page, here's what I think the alternatives for the release are, in order of my preference for them:

  1. Add the commit in this PR, which is very very late but the obvious fix to a know bug
  2. Use the current release candidate, which can sometimes produce a contract that evaluates to a runtime error. (It seems unlikely that any users would notice this bug, since it didn't show up in pkg-build)
  3. Revert the or/sc changes, keep that unsoundness for one more release (I think it was known since 2014)

Here are the commits to master that need to be removed to revert the or/sc change:

9df037b0f6e6b2df3cc0993631bb2870367e291a
d3efa4600342f9444507bb9d70137db06678c0bd
6cffbfa6d827b22a41a5b9daad7399c8c49f083a
a58fc276c9f1bc7be4c9cda6c82ce7e518d8fc05
cfa9918ac50a6888236fdbf67dcaf9ac18564234
26f65f13f75d8dad1b9d388388dd24030076a9c6

@bennn
Copy link
Contributor Author

bennn commented Oct 20, 2017

Startup times on master are similar --- a little slower than 6.10.1.
logs-master.txt

I also ran the tr-performance script, "master vs. 6.10.1" and "pr631 vs 6.10.1" are similar. (Note I have the wrong order of arguments to tr-performance, so when it says "old-time" that's actually referring to "master" or "pr631". And "new-time" is 6.10.1.)
perf-master.txt
perf-pr631.txt

The outlier from dungeon happened again :(
It happened 1 time in 10 during the first run of "pr631" just after running 6.10.1. I was able to reproduce it once using the same order of arguments (and the outlier is again the first run), but every other time I run there's no outlier.

@samth
Copy link
Member

samth commented Oct 21, 2017 via email

@bennn
Copy link
Contributor Author

bennn commented Oct 21, 2017

master bedroom?

I think I know what up with dungeon. That program uses math/array, so probably compile times are getting slow after I switched branches on Typed Racket.
(Because I usually do raco setup typed-racket and never thought to recompile other libraries.)

@samth
Copy link
Member

samth commented Oct 21, 2017

Damn autocorrect. I mean "master vs this PR".

@bennn
Copy link
Contributor Author

bennn commented Oct 22, 2017

Master's a little bit faster:

program master (stddev) pr631 (stddev) (/ master pr631)
acquire 10.28 (0.04) 10.26 (0.06) 1
dungeon 5.63 (0.03) 5.63 (0.03) 1
forth 3.14 (0.01) 3.14 (0.01) 1
fsm 3.2 (0.02) 3.18 (0.01) 1
math-flonum 2.51 (0.01) 2.52 (0.02) 1
new-metrics 2.12 (0) 2.13 (0.02) 1
old-metrics 1.61 (0.01) 1.61 (0.01) 1
parser 1.36 (0.01) 1.36 (0.01) 1
schml-interp-casts-help 14.14 (0.09) 14.36 (0.06) 1.02
schml-specify-rep 5.56 (0.02) 5.6 (0.09) 1.01

(I built this table from perf-master.txt and perf-pr631.txt, script attached
master-vs-pr631.rkt.txt

@samth
Copy link
Member

samth commented Oct 22, 2017

Great, this makes me think this is the right thing. I'll try to look at the actual code soon.

@samth
Copy link
Member

samth commented Oct 22, 2017

@bennn Is there a reason why reverting the or/sc change should require more than just reverting 9df037b?

@bennn
Copy link
Contributor Author

bennn commented Oct 23, 2017

No, I don't have a strong reason.
The commits I listed were all fixing things revealed by the or/sc change. If or/sc is reverted, the new code shouldn't do any harm. (I don't really believe that sentence, need to see what the tests say)


Commit-by-commit:

  • 9df037b is the or/sc change
  • d3efa46 adds a case to trusted-side-reduce so the type Syntax makes a flat contract
  • 6cffbfa changes Typed Racket's contract for hashtables and removes the constraint that hash/sc keys must be flat contracts.
  • a58fc27 is part of the changes to hash/sc
  • cfa9918 fixed a failed test related to hash/sc
  • 26f65f1 is part of the changes to TR's contract for hashtables

(I really wish I'd squashed those last 4)

@samth
Copy link
Member

samth commented Nov 7, 2017

Are we ready to go here?

@bennn
Copy link
Contributor Author

bennn commented Nov 7, 2017

LGTM, there's nothing more I want to add

@bennn bennn merged commit 9668453 into racket:master Nov 7, 2017
@bennn bennn deleted the issue-628a branch November 7, 2017 05:32
* Add `instantiate/optimize` to the static contracts API
  (new function in `instantiate.rkt`)
* Add optional kwd arg `#:recursive-kinds` to sc optimizer
* SC optimizer uses recursive kinds to tell if a `name/sc` or `recursive-sc`
  will generate a flat contract
* `instantiate/optimize`
  - solves for a recursive kinds table
  - calls `optimize` with the table
  - calls `instantiate` with the same table
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants