JRuby

Class and module names may need to be cached

Details

  • Type: Improvement Improvement
  • Status: Resolved Resolved
  • Priority: Blocker Blocker
  • Resolution: Fixed
  • Affects Version/s: JRuby 1.7.0.pre1
  • Fix Version/s: JRuby 1.7.0.pre1
  • Component/s: Core Classes/Modules
  • Labels:
    None
  • Number of attachments :
    0

Description

During the 1.7 dev cycle, I removed caching of module and class names due to the complexity of supporting their updates when classes and modules enclosing them have their names updated. Specifically, I could not find an efficient way for a class contained within an anonymous class to get a new, non-anonymous name when the anonymous container gains a name later on.

As a result, the performance of Class#name and Module#name has degraded in the 1.7 dev branch (master).

We could potentially still compensate for this degradation with a new form of caching. I have made such an attempt and posted it here: https://gist.github.com/1372628

I use the constant cache invalidation as a way to know that somewhere, some name has been updated. It does not pass tests, but I think it's possible for us to fix it.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Charles Oliver Nutter added a comment -

I now believe this could be a more serious problem. All entries into interpreted frames through ASTInterpreter methods capture the current class's name. This means that under JRuby 1.7, all interpreted frame entries are recalculating the entire name by walking the namespace hierarchy.

The name is only being used for the "Rubinius" stack trace, which shows the class that contains a given method (or the current class in the case of evals and class bodies). This does not affect Rubinius because they do not correctly update the name of a class when an anonymous containing class becomes no longer anonymous.

Another option for us would be to store a reference to the class in the backtrace element, only for as long as it is live on the stack...but this too seems like weak sauce.

In any case, I'm marking this a blocker for 1.7, since we can't pay the cost of recalculating the name for every interpreted frame entry.

JRUBY-6201 was impacted by a similar issue, where a class_eval's containing singleton class had its anonymous name recalculated every time. The performance impact there was fairly significant, but has been remedied by caching anonymous class names (since they never change). However, no other cases gained any caching.

Show
Charles Oliver Nutter added a comment - I now believe this could be a more serious problem. All entries into interpreted frames through ASTInterpreter methods capture the current class's name. This means that under JRuby 1.7, all interpreted frame entries are recalculating the entire name by walking the namespace hierarchy. The name is only being used for the "Rubinius" stack trace, which shows the class that contains a given method (or the current class in the case of evals and class bodies). This does not affect Rubinius because they do not correctly update the name of a class when an anonymous containing class becomes no longer anonymous. Another option for us would be to store a reference to the class in the backtrace element, only for as long as it is live on the stack...but this too seems like weak sauce. In any case, I'm marking this a blocker for 1.7, since we can't pay the cost of recalculating the name for every interpreted frame entry. JRUBY-6201 was impacted by a similar issue, where a class_eval's containing singleton class had its anonymous name recalculated every time. The performance impact there was fairly significant, but has been remedied by caching anonymous class names (since they never change). However, no other cases gained any caching.
Hide
Permalink
Charles Oliver Nutter added a comment -

Commit 5e13853 removes the need for class name in backtrace data, so at least that part of the problem is solved. There's still a need to consider caching class names, in case we run into additional cases.

That commit also added getSimpleName, which could (should?) be used for internal need to report some class name that may not need to be the fully-qualified, slow version.

Show
Charles Oliver Nutter added a comment - Commit 5e13853 removes the need for class name in backtrace data, so at least that part of the problem is solved. There's still a need to consider caching class names, in case we run into additional cases. That commit also added getSimpleName, which could (should?) be used for internal need to report some class name that may not need to be the fully-qualified, slow version.
Hide
Permalink
Charles Oliver Nutter added a comment -

For 1.7.0pre1, I reworked how class names are generated so that they can be safely cached.

Show
Charles Oliver Nutter added a comment - For 1.7.0pre1, I reworked how class names are generated so that they can be safely cached.

People

  • Assignee:
    Charles Oliver Nutter
    Reporter:
    Charles Oliver Nutter
Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: