Conversation
Check removed with assumption that only PROC will use it.
…g getSignature on NativeMethod
We have had a long-term goal to replace Arity with Signature (started in 2014?). The main issue is that Arity had no concept of keyword arguments. The math behind arity lives on via Ruby's arity method but the underlying structure needs more information. This commit completely deprecates all use of arity internally. There are a few removals of long deprecated code using Arity but we still have a lot of deprecated code just in case. For example, jruby-rack still depends on this on one constructor still which receives Arity. Anything which was using arity is now written in terms of Signature. The largest missing piece of DynamicMethod not having a getSignature. As mentioned in the last paragraph we only have a single live code path using Arity from jruby-rack. On the performance front we will do less work (although I doubt we will see much we can measure). In startings up Rails server on a single controller app we were doing 3k arity calcs and over 400k signature calcs. Now we are doing no arity calcs and only 2k signature calcs. With some better mechanism for Signature creation we could get this down to a few hundred. The second aspect of this is reducing memory about 1Mb for the same Rails app. Signature is about 100k now but since it is immutable we can reduce this down to close to 0 if we share all combos during creation. The more near term motivation was seeing a mixture of logic between arity and signature involved in block argument processing. This branch has already removed some complexity of block processing but every extra detail we need to keep in our head makes fixing block issues that much harder.
kares
approved these changes
Mar 2, 2021
Member
kares
left a comment
kares
left a comment
There was a problem hiding this comment.
👍 nice to see the deprecated Arity param methods removed
Member
Author
|
@kares honestly there are a ton which should be but jruby-rack popped out as a currently using Arity consumer and I wonder if there are others. So I removed a few which seemed unlikely to be used and got frightened of removing more. We really need a nice way of auditing all the external java exts. |
headius
reviewed
Mar 2, 2021
| @@ -1,4 +1,5 @@ | |||
| /***** BEGIN LICENSE BLOCK ***** | |||
| /* | |||
| **** BEGIN LICENSE BLOCK ***** | |||
headius
approved these changes
Mar 2, 2021
Member
headius
left a comment
headius
left a comment
There was a problem hiding this comment.
Nice cleanup. Make sure any unused Arity imports have been cleaned out.
| @@ -1,4 +1,5 @@ | |||
| /***** BEGIN LICENSE BLOCK ***** | |||
| /* | |||
| **** BEGIN LICENSE BLOCK ***** | |||
| @@ -1,4 +1,5 @@ | |||
| /***** BEGIN LICENSE BLOCK ***** | |||
| /* | |||
| **** BEGIN LICENSE BLOCK ***** | |||
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This branch was originally opened to solve some block problems but I quickly ran into use continuing to use Arity over Signature.
So nearly all of this is removing use of Arity from our code base (jruby-rack still uses a single Arity path in our codebase).
There are two block changes as well:
As for Signature code migration it is large but pretty straight-forward. We calculate arity values for populators and we probably could do that differently in a post-Arity world but that is a topic for another day. The perf aspect is worth repeating (from 9ccc321):
"On the performance front we will do less work (although I doubt we will
see much we can measure). In startings up Rails server on a single
controller app we were doing 3k arity calcs and over 400k signature calcs.
Now we are doing no arity calcs and only 2k signature calcs. With some
better mechanism for Signature creation we could get this down to a few
hundred. The second aspect of this is reducing memory about 1Mb for the
same Rails app. Signature is about 100k now but since it is immutable we
can reduce this down to close to 0 if we share all combos during creation."
The other thing which could/should happen post commit is to make a better mechanism for sharing the same signatures so we create less instances. Right now a small Rails app makes 100k heap worth of Signatures but there are probably only a hundred or two hundred signatures which could exist (note: this was much larger number before this work). That would make the heap size very small. As it is this is about 0.1% of the heap for the Rails app. Making it 0.01% is nice but not worth a lot of effort.