-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
cmd/compile: introduce CINT64 to reduce memory footprint #4617
Comments
yes. cmd/gc now allocates a Mpint (16 words) for each number in source, we should be able to share them or even use a smaller representation if suffice; I've tried this approach, but it changed so many things. I propose we introduce one extra level of indirection for Mpint, something like this: union { struct realMpint *p; long taggedWord; // last bit is 1 for a 31-bit or 63-bit integer } Mpint; and use this to replace Mpint* in the Val struct. or, alternatively, we can share Mpint*, but that still a big change as we must take care when changing the shared copy. Any thoughts on this? |
for Mpflt, we can even use NaN-boxing to hide a pointer to realMpflt inside a normal double, however, this might be too hacky (it should be portable as we do require IEEE-754 floating point, and the only problem i can think of is that Solaris sometimes return addresses in the upper half of amd64 virtual address space). |
Thank you for your fast reply re #1: is there a simple way I could instrument the compiler to tell how many Mpints would fit into a long, or would require indirection to a readMpint ? My suspicion is the number would be quite small, so this patch would turn out to be a net positive for almost all compilations. Re: sharing *Mpints, I did a very quick summary of the different numbers in tables.go (see attached), and while there is a lot of duplication, there are close to 200k lines in the file, unless sharing an Mpint cost very little, I don't think this would save memory, or time. Attachments:
|
Mpflt contains a high-precision floating point number. It is not IEEE in any way. There are no NaNs. Two possibilities that come to mind: 1) Add a new val.ctype = CTINT64 and add a int64 field to the val union. Make sure that CTINT is only ever used for values that do not fit in an int64. 2) In lex.c, at the ncu: label, before allocating an mpint, look in a hash table (can reuse lookup probably) to see if we've seen this number before, and if so, reuse the Mpint from the last time. (They are not modified once craeted.) I think (1) is more work but will end up significantly cleaner and more robust. I think (2) is a small amount of work but might introduce subtle bugs and may not work at all. Labels changed: added priority-later, removed priority-triage. Status changed to Accepted. |
I have tried 2) once, but it didn't work due to a mutation that happened later, but maybe I was just doing wrong. 2) would be quite effective on rotate.go where we have many many small literals, but I think the more common use case is the collate one, where we have many small-sized literals, but usually all different (or in cryptography/numerical computations/video encoding with precomputed, possibly large tables). So I think 1) is useful for large precomputed tables with non-trivial values, 2) is useful for large auto-generated code with many small constants (but then the size of the AST dwarfs the size used for literals). |
Please only introduce CINT64. CFLT64 will be a lot of work and almost never used. Remember that constants need to be ideal and high-precision. Even if you can store one particular floating-point value 100% accurately in a float64, any mathematical operation on it will need significant work to figure out whether the result also fits exactly in a float64, and it almost never will. Russ |
just for clarification: I don't mean this issue is not important, and in fact, saving ~16MiB is well worth doing. I just want to know whether there is other (potentially bigger) memory saving possibilities for issue #1860 and the original exp/locale/collate/tables.go issue. |
(pprof) top Total: 510.8 MB 409.5 80.2% 80.2% 409.5 80.2% nod 53.9 10.5% 90.7% 54.0 10.6% _yylex 22.9 4.5% 95.2% 22.9 4.5% prog 6.8 1.3% 96.5% 28.3 5.5% nodintconst 6.0 1.2% 97.7% 6.0 1.2% list1 5.8 1.1% 98.8% 5.8 1.1% entry 2.8 0.5% 99.4% 2.8 0.5% typ 1.5 0.3% 99.7% 1.5 0.3% inithash 0.8 0.1% 99.8% 0.8 0.1% rega 0.4 0.1% 99.9% 0.4 0.1% pkglookup Attachments:
|
Issue #5483 has been merged into this issue. |
Now that the compiler is in Go, and we have a nice union type for constant values, and there's a renewed concern about compiler speed, adding CTINT64 might be worth another look. |
I started evaluating replacing Mpint by several type implementations: one for CTRUNE and CTINT and an implementation for each one using an int64 and one using a big.int. I have gotten as far as replacing Mpint by an interface and changing the compiler not to depend on internal implementation details of CTINT/CTRUNE types. I also separated out the the CTRUNE type from Mpint by embedding an Mpint into a new Mprune type. My next step is to make an Integer interface satisfying type that uses only a int64 for storage and returns an Mpint which uses big.int on overflow when operated on. Ones i have gotten as far producing a working prototype that shows memory savings i will write another update with benchmark numbers. |
@martisch Have you considered go/constant? We could just use it, or borrow from it heavily. |
@griesemer go/constant looks like it could be used as a drop in replacement (with some extensions) for gc Val when the compiler does not depend on implementation details of Val anymore. go/constant does not seem to support an extra rune type or nil value type. We might not need the type constants CTINT, CTRUNE, ... as such after all and can just differentiate on the Kind of value directly always. I will try out a CL first that just uses an texteneded go/constant and modify all the users in the compiler to use the new Val interface together with some helper functions. Will write back here if i encounter any not yet known problems with that approach. |
@martisch A few things to consider:
|
agreed. seems better to keep it pure and store the ideal type information in an extra field besides the constant value. This also should allow to use the go/constant package without further modifications. |
Change https://golang.org/cl/272654 mentions this issue: |
The text was updated successfully, but these errors were encountered: