-
Notifications
You must be signed in to change notification settings - Fork 262
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
Use native byte/char arrays in Go #2818
Conversation
This solution uses an AutoGeneratedToken when filling in an omitted size. Previously, the code looked at the LiteralExpr in ArrayDimensions[0], which had the effect of also eliding a programmer-supplied size. Also, the previous implementation would crash if a size was given but wasn’t a LiteralExpr.
* Remove duplicate check * Add omitted check * Change error message to talk about “memory limit” rather than some C# property
Just to update: I've been working on fixing the remaining issues with the new |
The Java fixes are rather involved so I'm going to move them to a separate PR, and just merge this one with Java disabled in the new @fabiomadge would you mind giving my two commits (6835539 and bcd2f04) a sanity check before I merge this? Rustan asked me to add whatever was necessary to complete this while he was on vacation, but I don't want to use that to sneak code in without review. :) |
@@ -1665,7 +1661,7 @@ private class ClassWriter : IClassWriter { | |||
SeqType or MultiSetType => ("[", "]"), | |||
_ => ("{", "}") | |||
}; | |||
wr.Write(TypeHelperName(ct)); | |||
wr.Write(ct is SeqType ? DafnySeqMakerFunction : TypeHelperName(ct)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you try doing this without circumventing the TypeHelperName
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did at first and found the code generation was a lot messier.
Source/DafnyRuntime/DafnyRuntime.py
Outdated
if self.isStr is None or other.isStr is None: | ||
isStr = None | ||
else: | ||
isStr = self.isStr and other.isStr | ||
return Seq(super().__add__(other), isStr=isStr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure that this changes the semantics?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. Before Seq("a", isStr=None) + Seq("b", isStr=None)
would become the same as Seq("ab", isStr=False)
, which would still go through the inference and decide it was a string, which we don't want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
>>> (None and None) == None
True
>>> (None and None) == False
False
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah you're right, self.isStr = None
is all that needed to change. I added this at the same time and never tested it without. :) Thanks!
Source/DafnyRuntime/DafnyRuntime.py
Outdated
@@ -399,6 +402,12 @@ def plus_char(a, b): | |||
def minus_char(a, b): | |||
return chr(ord(a) - ord(b)) | |||
|
|||
def plus_unicode_char(a, b): | |||
return CodePoint(plus_char(a, b)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was certain to have made this remark elsewhere, but can't find it, so: Can you overload +
and -
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah true, I can do that. For the record I don't care much about the usability of CodePoint
though, since I only expect compiled Dafny code to reference it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It did end up simplifying the code generation logic a little though, which I DO care about :) , so thanks!
Co-authored-by: Fabio Madge <[email protected]>
The Python changes look ok to me. |
This PR does three things to improve performance of Go code that uses arrays: * For nonempty arrays where the target representation of elements uses `uint8` or `rune`, the underlying `Array` uses a `[]uint8` or `[]rune` array, respectively, instead of the default `[]any`. * For 1-dimensional arrays, the index is passed directly to the `ArrayGet`/`ArraySet` method in the Dafny runtime, rather than using a var-args version of those methods. * When the static type of the array operations reveals that the underlying representation is `uint8` or `rune`, then special versions of `ArrayGet`/`ArraySet` are called where the argument/return is a `uint8` or `rune`, respectively, rather than the default versions, which use an `any` (that is, `interface{}`) as the argument/return. Unlike the current compilation to Java, which introduces type descriptors for all types in order to specialize arrays, this PR uses existing type descriptors or the initialization elements provided by the program. The one case where the type cannot be recovered for specialization is for 0-length arrays, which this PR represents with an underlying `nil` array. It seems the design in this PR can be adapted to Java or JavaScript as well. The performance improvements are as follows. The program run is included below, and the Go measurements were repeated 6 times (each varying less than 0.2s from the numbers shown here). | target features | time | | ------------- | ------------- | | C# | 31.4s | | Go with specialized arrays and no boxing across Set/Get | 27.4s | | Go with specialized arrays, but still using boxes across Set/Get | 31.8s | | Go with arrays of `any` | 103.6s | | Go with arrays of `any` and using var-args even for 1-dim arrays | 179.1s | ``` dafny newtype byte = x | 0 <= x < 256 newtype int32 = x | -0x8000_0000 <= x < 0x8000_0000 method MM<X>(x: X) returns (r: X) { r := x; } method Main() { for i := 0 to 10_000 { var a := GenerateArray(); a := CopyArray(a); var s := SumArray(a); expect s == 0; } print "done\n"; } method GenerateArray() returns (b: array<byte>) ensures b.Length == 256_000 { b := new byte[256_000]; for i: int32 := 0 to 256_000 { b[i] := (i % 256) as byte; } } method CopyArray(a: array<byte>) returns (b: array<byte>) requires a.Length == 256_000 ensures b.Length == 256_000 { b := new byte[256_000]; for i: int32 := 0 to 256_000 { b[i] := a[i]; } } method SumArray(a: array<byte>) returns (sb: byte) requires a.Length == 256_000 { var s: int32 := 0; for i: int32 := 0 to 256_000 invariant 0 <= s < 256 { s := s + a[i] as int32; if 256 <= s { s := s - 256; } } sb := s as byte; } ``` ## Other fixes along the way As part of writing tests for the new functionality, I detected and fixed various other infelicities as well: * fix: In Java, cast to non-box type after let * fix: In JavaScript, compare arrays by comparing their references, not their elements (issue dafny-lang#3207) * fix: In C#, add some necessary casts * In Go, improved implementation of array comparisons * In Go, fixes array comparisons (to be reference equality, not equality of elements) (dafny-lang#2708) * fix: In Java, fix enumerations of `long` Java values (which previously ended up truncating numbers to `int`s) * fix: In Java, don't confuse a Dafny-defined type `Long` with Java's `java.lang.Long` (and similar for other integer types) (reported as dafny-lang#3204) * fix: In Python, don't share inner arrays, which was previously done when arrays have 3 or more dimensions * Across the target languages, pass BigInteger as array sizes and use native integers for array indices * Pretty printing: Elide the array size in `new` only when the input program does (this uses `AutoGeneratedToken`, as in other places) * When there are several `IntBoundedPool`s, try a little harder to pick the best bounds. Alas, there are still simple cases where the target code ends up enumerating all 32-bit integers, for example. To improve the target code further, we need to make use of the partial-evaluation of expressions, which exists elsewhere in the Dafny implementation, and/or make use of run-time `Min` and `Max` routines. Fixes dafny-lang#3204 Fixes dafny-lang#2708 Fixes dafny-lang#3207 <small>By submitting this pull request, I confirm that my contribution is made under the terms of the [MIT license](https://github.com/dafny-lang/dafny/blob/master/LICENSE.txt).</small> Co-authored-by: Fabio Madge <[email protected]> Co-authored-by: Robin Salkeld <[email protected]> Co-authored-by: Aaron Tomb <[email protected]>
Fixes #3413. Addresses the issues uncovered during #2818 by adding a `--unicode-char` mode version of `Test/comp/Arrays.dfy`, which all stem from incomplete handling of manual boxing/unboxing of the `CodePoint` type at various Java code generation points. Note this is still incomplete as there must be other spots that do not handle coercion correctly in general, but these changes at least cover `Arrays.dfy`. It turned out that handling arrow coercion as in the Go compiler was not necessary for Java, because the initial casting of a function reference as a lambda is where the necessary boxing/unboxing needs to happen instead anyway. That is, a reference to a Dafny `function f(x: char): char` has to end up as a Java `Function<CodePoint, CodePoint>` and therefore be eta expanded from the start. Also removed the fail-fast behavior from `%testDafnyForEachCompiler` as I found it more useful for debugging that way. Also implemented `ConcreteSyntaxTree.Comma` as part of an initial implementation of Java arrow conversion, which ended up not directly used but seems useful enough to keep (and I refactored one spot to use it as an example). Edit: I've also added a few more similar cases exposed as part of enabling `/unicodeChar:1` by default for Dafny 4.0: #3635 Co-authored-by: Rustan Leino <[email protected]> Co-authored-by: Fabio Madge <[email protected]> Co-authored-by: Aaron Tomb <[email protected]>
This PR does three things to improve performance of Go code that uses arrays:
uint8
orrune
, the underlyingArray
uses a[]uint8
or[]rune
array, respectively, instead of the default[]any
.ArrayGet
/ArraySet
method in the Dafny runtime, rather than using a var-args version of those methods.uint8
orrune
, then special versions ofArrayGet
/ArraySet
are called where the argument/return is auint8
orrune
, respectively, rather than the default versions, which use anany
(that is,interface{}
) as the argument/return.Unlike the current compilation to Java, which introduces type descriptors for all types in order to specialize arrays, this PR uses existing type descriptors or the initialization elements provided by the program. The one case where the type cannot be recovered for specialization is for 0-length arrays, which this PR represents with an underlying
nil
array. It seems the design in this PR can be adapted to Java or JavaScript as well.The performance improvements are as follows. The program run is included below, and the Go measurements were repeated 6 times (each varying less than 0.2s from the numbers shown here).
any
any
and using var-args even for 1-dim arraysOther fixes along the way
As part of writing tests for the new functionality, I detected and fixed various other infelicities as well:
long
Java values (which previously ended up truncating numbers toint
s)Long
with Java'sjava.lang.Long
(and similar for other integer types) (reported as Emitted Java converts long to int and loses precision #3204)new
only when the input program does (this usesAutoGeneratedToken
, as in other places)IntBoundedPool
s, try a little harder to pick the best bounds. Alas, there are still simple cases where the target code ends up enumerating all 32-bit integers, for example. To improve the target code further, we need to make use of the partial-evaluation of expressions, which exists elsewhere in the Dafny implementation, and/or make use of run-timeMin
andMax
routines.Fixes #3204
Fixes #2708
Fixes #3207
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.