Rack rack → 1.1

Comments and changes to this ticket

• 

  chneukirchen (at gmail) December 30th, 2008 @ 11:13 AM

  Anyone have benchmarks how much this helps?

  If it's significant, we can apply it of course.

• 

  josh January 15th, 2009 @ 09:32 PM

  • State changed from “new” to “invalid”

  Invalidating until we have some real numbers.

• 

  josh August 3rd, 2009 @ 04:29 PM

  • State changed from “invalid” to “open”
  • Milestone changed from “0.9” to “1.1”
• 

  josh August 3rd, 2009 @ 09:05 PM

  • State changed from “open” to “resolved”
• 

  raggi August 5th, 2009 @ 03:04 PM

  • State changed from “resolved” to “open”

  This diff broke the documentation.

  e.g.

    #  app = Rack::Builder.app do
    #    use Rack::CommonLogger
    #    lambda { |env| [200, {Const::CONTENT_TYPE => 'text/plain'}, 'OK'] }
    #  end
  

  Please do not use find and replace for refactors, it is not appropriate.

  Furthermore, I would recommend several further corrections to this:

  1. Remove the use of freeze. There's no good reason. Defensive programming is not a good reason, even if we are a framework / toolkit.
     
  2. Use a conventional mapping from filename -> constant name. In other words, please call Const Constants.
     
  3. If you don't want to litter the code with Constants::, please use include Rack::Constants in relevant files. This also means the code is more portable, and does not implicitly rely on being within the Rack module. (More important for middleware stuff that might one day be moved to something like rack-contrib).
• 

  chneukirchen (at gmail) August 5th, 2009 @ 03:29 PM

  Josh, do we have numbers on the usefulness of this now? I haven't seen any so far.

  raggi, ack to all of your points.

• 

  josh August 5th, 2009 @ 04:09 PM

  • State changed from “open” to “invalid”

  reverted, this isn't worth my trouble

• 

  josh August 6th, 2009 @ 01:30 PM

  Freezing keys matters: http://gist.github.com/163310

  Otherwise this Const hackery creates more objects as a result.

  If you want to reconsider, one of you needs to redo it.

• 

  raggi August 6th, 2009 @ 02:19 PM

  "Otherwise this Const hackery creates more objects as a result." - I don't see the evidence for this, maybe I'm not looking at it the right way, can you clarify for me?

  Please don't take offence at the challenge, any challenge is expectant of discussion, and I'm more than willing and happy to listen, and also to be wrong.

• 

  raggi August 6th, 2009 @ 02:27 PM

  Some more food for thought, these benches should give you some idea of other impacts. Pay particular attention to proportional statistics between null time and runtimes, and also the iteration level. Combine that with the use frequency in the use case for the relative benefits.

  http://gist.github.com/162877

  http://gist.github.com/162872

  It's also worth paying attention to the differences between the interpreters.

  What also might be considered, is moving to symbol keys, but that would be a major API change.

• 

  josh August 6th, 2009 @ 03:11 PM

  +all my votes on moving to symbols

  The whole point of this const hackery is because we want to fake "atomic immutable" keys.

  My we do the HashWithIndifferentAccess thing for env and store things internally as symbols and prefer symbol lookup over string. I think the env key set is pretty finite and shouldn't cause to many symbol memory leak problems. Just need to warn people, don't store "foo1", "foo2" crap directly on the env hash.

  It should be likely that the initially inserted key and lookup key are the same object since we are using the two in the adapters and in middleware and request objects.

• 

  raggi August 6th, 2009 @ 03:50 PM

  The reason I think symbol keys is less desired is that many people consider the following thing to be really ugly:

  :'rack.errors'

  I understand a motivation for the desired side effects of atomic immutable keys, however, desiring a side effect and measuring what it actually gains you, vs. what it pains others, is highly relevant for such decisions. As such, it is this notion that we must put under the most significant discussion.

  It should be noted that servers will also have to be corrected to take advantage of what you showed in your benchmark as advantageous. I must admit that I predicted exactly that, and indeed mentioned it to chris yesterday. I wanted to see what you were measuring before I posted my sanity check benchmarks here, and I was right in both my predictions and my analysis as far as I can see.

  With regard to the final comment on insertion key and lookup key, I disagree. Mongrel does not rely on rack (meaning it will never use the constants), and passes in most of the env, which rack passes through mostly without touching it. (http://github.com/rack/rack/blob/71030b9dbea07d4e573803805ff430d52a.... In this case you loose the performance gains you want through lack of atomicity.

  With the above being the case, the average gain is the difference between the last two cases in this benchmark: http://gist.github.com/162872. The actual performance gain from this is an average of about 0.01/2000000s per access, which is generally considered to be statistically irrelevant. Furthermore being an interpreter based special case, you will notice than on jruby, it is actually slower, where also the marked GC affects of atomising these strings is also less important due to a significantly more performant and configurable GC. Furthermore, for long lived programs on MRI, the new 1.9.2 heap patches may also render the memory usage portions of these optimisations irrelevant also.

  Further discussion welcome, as always.

• 

  josh August 6th, 2009 @ 04:09 PM

  lets just leave the patch as invalid and wait on VMs to fix the issue