-
-
Notifications
You must be signed in to change notification settings - Fork 925
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[Truffle] Cache of integers formatted as ropes.
- 9.4.12.0
- 9.4.11.0
- 9.4.10.0
- 9.4.9.0
- 9.4.8.0
- 9.4.7.0
- 9.4.6.0
- 9.4.5.0
- 9.4.4.0
- 9.4.3.0
- 9.4.2.0
- 9.4.1.0
- 9.4.0.0
- 9.3.15.0
- 9.3.14.0
- 9.3.13.0
- 9.3.12.0
- 9.3.11.0
- 9.3.10.0
- 9.3.9.0
- 9.3.8.0
- 9.3.7.0
- 9.3.6.0
- 9.3.5.0
- 9.3.4.0
- 9.3.3.0
- 9.3.2.0
- 9.3.1.0
- 9.3.0.0
- 9.2.21.0
- 9.2.20.1
- 9.2.20.0
- 9.2.19.0
- 9.2.18.0
- 9.2.17.0
- 9.2.16.0
- 9.2.15.0
- 9.2.14.0
- 9.2.13.0
- 9.2.12.0
- 9.2.11.1
- 9.2.11.0
- 9.2.10.0
- 9.2.9.0
- 9.2.8.0
- 9.2.7.0
- 9.2.6.0
- 9.2.5.0
- 9.2.4.1
- 9.2.4.0
- 9.2.3.0
- 9.2.2.0
- 9.2.1.0
- 9.2.0.0
- 9.1.17.0
- 9.1.16.0
- 9.1.15.0
- 9.1.14.0
- 9.1.13.0
- 9.1.12.0
- 9.1.11.0
- 9.1.10.0
- 9.1.9.0
- 9.1.8.0
- 9.1.7.0
- 9.1.6.0
- 9.1.5.0
- 9.1.4.0
- 9.1.3.0
- 9.1.2.0
- 9.1.1.0
- 9.1.0.0
1 parent
7e7f844
commit 7d82684
Showing
2 changed files
with
30 additions
and
2 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7d82684
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.
@nirvdrum seems like a good idea to cache this.
7d82684
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.
@chrisseaton I think this will lead to trouble: yes, GC will collect
LeapRope
string objs from the weak reference, but the key-value mapping (and its associatedMap.Entry
) will stick around, leaking memory.Are you trying to tackle the issue of
to_s
Strings always escaping (due to EA non being able to handle var-length objs)? If so, it might be worth it to profile max length of strs and allocate fixed-widthRope
s, the same way you do it for arrays?DynamicObj
+Rope
(+byte[]
inRope
) is quite heavy already, so an overhead of a few unused bytes might be worth EA-ability?7d82684
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.
Yes I suppose it's quite likely someone could run in a loop formatting random integers for a long time.
The EA can handle variable length objects though - all our argument arrays are EAd. I was just trying to cache what I thought would be common objects. At the moment going from
Integer
toRope
I think meansInteger -> byte[] -> char[] -> byte[]
, so it's not ideal to be happening a lot, especially in cases like backtraces where they're probably never even seen most of the time.Maybe what I really wanted was a
FormatIntegerRope
. Leave the numbers in there lazily. If they're just created and then printed to a file stream or something we can delay the formatting a long way.7d82684
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.
@headius @enebo from your experience, isn't it the case, that this something that is almost certainly guaranteed to happen?
This is probably because during PE/compilation, the args array looks fixed-length to Graal. AFAIK hotpost requires compiled methods to have fixed stack size, so var-length EA is not feasible right now.
Back to my original suggestion. If the return value of
Fixnum#to_s
doesn't escape, EA will removeDynObj
+Rope
(+Integer.toString
'sString
obj) allocations, leaving behind only a tinychar[]
, so my fixed-width profile-driven idea is almost certainly not worth it.7d82684
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.
After some thought, consider e.g. Rails's ORM: there will be a ton of
Product.find(params[:id])
calls, whereparams[:id]
is a HTTP provided string, that will be parsed into aFixnum
(for security purposes) and then subsequently internally used to generate a SQL query:"SELECT * FROM products WHERE id = #{id}"
...PS: sorry for spamming everybody.
7d82684
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.
Yes I think I'll try the lazy integer node method. I won't leave this cache in place as in as it is not limited by anything.
7d82684
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.
One could also locally cache with
@Cached
in Fixnum#to_s. It would probably be interesting to rewrite Integer.toString ourselves to be more PE-friendly. If it turns out multiple integers output a String of the same length, it might allow interesting optimizations further.7d82684
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.
We're flying blind here - I'll produce some benchmarks to show what I'm trying to do.