#13 ✓invalid

Add common HTTP strings to Rack::Const

Reported by josh | December 30th, 2008 @ 05:30 AM | in 1.1

Add Rack::Consts module to hold common HTTP strings to reduce garbage used by Rack libs.

I'm sure the Merb guys will back up the performance benefits :)

Comments and changes to this ticket

  • chneukirchen (at gmail)

    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

    josh January 15th, 2009 @ 09:32 PM

    • State changed from “new” to “invalid”

    Invalidating until we have some real numbers.

  • josh

    josh August 3rd, 2009 @ 04:29 PM

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

    josh August 3rd, 2009 @ 09:05 PM

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

    raggi August 5th, 2009 @ 03:04 PM

    • State changed from “resolved” to “open”

    This diff broke the documentation.


      #  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)

    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

    josh August 5th, 2009 @ 04:09 PM

    • State changed from “open” to “invalid”

    reverted, this isn't worth my trouble

  • josh

    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

    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

    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.



    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

    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

    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:


    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

    josh August 6th, 2009 @ 04:09 PM

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

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile ยป