racket / ChezScheme

Chez Scheme
Apache License 2.0
110 stars 8 forks source link

Fix race condition in cptypes #18

Closed gus-massa closed 4 years ago

gus-massa commented 4 years ago

The former changes introduced a (few?) race condition in cptypes that caused many errors, in particular when the special case for call-with-values was enabled. After a lot of help from @mflatt I think I fixed it.

The first commit replace the implicit counter used to number the prelex with an explicit one. This solves the problem in the multithread version, and it is possible to enable the reduction for call-with-values.

Most of the other commits, move the calculations of the types of the result and arguments of the primitives from cptypes to priminfo, so they are calculated once. This removes all the thread specific data and is important for the next step. Also, it enables to remove the signature field from the primref records. As a side effect, removing the signature from primref fix an error reported in the mat compute-size-incremets in misc.ms because the threads were using too much memory.

The last commit moves more of the internal function from the body of cptypes to the surrounding let. In particular, with this change the closures for the specialized cases are calculated only once at the beginning and not again in every call to cptypes.

To keep the signatures consistent, I changed the result of a few of them from ptr to void/list or something like that. Perhaps it would be better to change them just to true.

The new code is more picky about the signatures, but it includes all the current cases. (I just had to reorder one.)

The only case that the new code can't reduce that was reduced in the past is

(lambda (x) (atan x) (number? x)) ; ==> (lambda (x) (atan x) #t)

and the equivalent cases with two arguments. Anyway, I don't expect it to be a problem and I plan to add a special case for it in the future.

mflatt commented 4 years ago

This looks great.

It also looks like we'll need to sync these commits with a Racket version bump (mainly due to primvar changing back). That means

The changes in the ChezScheme and racket repos need to be pushed at the same time.

I'm happy to do these steps at the point where you're ready for this to be merged, so let me know.

gus-massa commented 4 years ago

I collapsed the changes in three commits, rebased, and added the changes in the version. For the racket part I wrote this https://github.com/gus-massa/racket/commits/20-3-FixCptypes

Are they OK? If not, I can revert to the old version.

mflatt commented 4 years ago

Yes, that all looks good and ready to merge in both repos. Thanks!