#40 ✓resolved
Chris Heald

[PATCH] [Optimization] Rack::URLMap clones environment hash

Reported by Chris Heald | March 22nd, 2009 @ 05:44 AM

Very simple little change. Rack currently clones the passed-in environment hash via env.merge, and then passes that to @app.call. #merge! is perfectly applicable here, and prevents an extra copy of the environment hash from hanging around in memory.

Comments and changes to this ticket

  • manveru

    manveru March 23rd, 2009 @ 03:04 AM

    • Tag set to env, lookup, performance, review, urlmap


    Hash#merge! is no good idea, I would like to defend the status quo. Doing a destructive update of the original env would lead to wrong routing if you are using more than one urlmap in a cascade and the app routed to returns a 404.

    So you would go from /app1/foo, given that there exists an app1, this part would be cut down to /foo, but if app1 returns 404 and the next urlmap kicks in it won't get the original anymore and repair is very hard.

    The current Hash#merge comes from one of my patches: http://github.com/rack/rack/comm...

    commit df37a917200a6d954292fe00a1b1fe1caa4afcfa
    Author: Michael Fellinger <m.fellinger@gmail.com>
    Date:   Thu Jan 29 22:34:43 2009 +0900
        Non-destructive URLMap
  • Chris Heald

    Chris Heald March 23rd, 2009 @ 05:02 AM

    Mmm, makes sense. Chalk this one up to a lack of understanding as to how URLMap could be chained then.

    For what it's worth, I discovered this when trying to track down why environment hashes weren't becoming eligible for collection in a Rails 2.3.2 application. I haven't answered that question yet, but I suspect that answer will be far more interesting.

  • manveru

    manveru March 24th, 2009 @ 03:15 PM

    • State changed from “new” to “resolved”
    • Assigned user set to “manveru”

    OK, I'll close this one for now.

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 ยป

People watching this ticket