`rqCaptureParams` for captured variables? #168

Open
favonia opened this Issue Feb 11, 2013 · 16 comments

Projects

None yet

3 participants

@favonia
favonia commented Feb 11, 2013

I'm wondering if we can have a new field rqCaptureParams in the Request and two separate functions like rqCaptureParam and rqCaptureParams to access them. I want to make sure, in the handler, that the user is indeed following the rule. I guess this can be done by copying the map in the function route' to the field in Request.

Related issues: The documentation of rqParams should mention variable capturing as well.

Thanks! I really like the overall design of Snap.

PS: My English sucks and so I'm not suggesting they should be named as above.

@favonia favonia added a commit to favonia/snap-core that referenced this issue Feb 11, 2013
@favonia favonia Proof of concept for Issue #168. b0bbf0a
@favonia favonia added a commit to favonia/snap-core that referenced this issue Feb 11, 2013
@favonia favonia Proof of concept for Issue #168. 11e920e
@gregorycollins
Member

I'm not so sure about this, let me think about it. Do we want to pay the overhead of yet another map on every request? What's the use case here?

@ghost
ghost commented Feb 13, 2013

How much overhead is it? Are we talking about the overhead of carting around an extra Map.empty? Or having to union an extra map when using the combined params map?

The use for this is just being able to tell the difference between me capturing something from the path as "id" and a user tacking "?id=whatever" onto the url. I prefer being able to validate the "id" or whatever when I capture it from the url, rather than having to validate it when pulling it out of the params map. With the current implementation, there's no way to tell whether the "id" I am pulling out is from the path or from a get or post param, so I have to validate it when I pull it out.

@gregorycollins
Member

Well, technically you can validate it because upon entry to a handler, any variable that appears in rqParams but not in rqQueryParams/rqPostParams must have come from URL capture. But that's a little weaselly.

What you would be paying for here per-request is:

  • extra machine word for the map pointer itself
  • if the map is non-empty, an extra 4-6 words for each key-value pair
  • extra CPU for computing the union of rqCapturedParams into rqParams (only paid on variable capture)

I would like to see some benchmark numbers with/without this change, both for a route without any captures and for routes with 4-5 captures. Would someone like to volunteer to produce those? If the difference is <5% I think we will be okay with merging this change. See https://github.com/snapframework/snap-benchmarks for the general testing methodology we have used in the past.

@favonia
favonia commented Feb 14, 2013

There is no extra overhead in computing the union. The map is constructed in the internal function route', merged into rqParams and then discarded in the current codebase. My patch simply stops the GC from recycling it. The possible computing overhead will be more GC time, more swapping, etc. In other words, I think we only have to worry about the extra memory overhead.

I've never done a serious benchmark before. Might need some more time if no other volunteers.

@gregorycollins
Member

Actually, after looking at it again, your patch as written is slightly
incorrect: you need to union into rqCaptureParams also to handle the
following case:

route [("foo/:bar", route [(":baz", quux)])]

As written the "bar" capture would get blown away when evaluating "quux".

@gregorycollins
Member

Another thing: for us to accept this patch you will need to add tests for the new behaviour. This patch almost certainly breaks the test suite, and we'll want to add tests in test/suite/Snap/Internal/Routing/Tests.hs to make sure the new code works as advertised. Please make sure to test the nested route case.

@favonia
favonia commented Feb 14, 2013

Oops I didn't notice the nested case! Sure I'll fix it. However, I might let route' start with the existing rqCaptureParams to avoid computing the union in the end. I planned to update the patch and add new test cases this weekend (when I can continue to work on my hobby server).

Question: The variables captured by the inner route will be in front of the list, right?

PS: I've already updated the existing test cases (by adding Map.empty) in the latest pull request. The test suite says the patch won't affect other functions. :)

@gregorycollins
Member

However, I might let route' start with the existing rqCaptureParams to avoid computing the union in the end.

I don't think that would work either; in the nested case captured params will get unioned with rqParams multiple times if you do that.

Question: The variables captured by the inner route will be in front of the list, right?

Not 100% sure, you'd have to double-check that.

@favonia
favonia commented Feb 14, 2013

I don't think that would work either; in the nested case captured params will get unioned with rqParams multiple times if you do that.

You're right. We really have to compute the unions. 😟

@favonia favonia added a commit to favonia/snap-core that referenced this issue Feb 18, 2013
@favonia favonia Proof of concept for Issue #168. ef091a2
@favonia favonia added a commit to favonia/snap-core that referenced this issue Feb 18, 2013
@favonia favonia Proof of concept for Issue #168. 311795e
@favonia favonia added a commit to favonia/snap-core that referenced this issue Feb 18, 2013
@favonia favonia Proof of concept for Issue #168. fb5a334
@favonia favonia added a commit to favonia/snap-core that referenced this issue Feb 18, 2013
@favonia favonia Proof of concept for Issue #168. f1dec17
@favonia favonia added a commit to favonia/snap-core that referenced this issue Feb 18, 2013
@favonia favonia Proof of concept for Issue #168. 0701442
@favonia
favonia commented Feb 18, 2013

@gregorycollins I'm not sure if I'm on the right path to run the benchmark. It doesn't compile (with GHC 7.6) because Prelude shows up in both base and haskell98. Apparently the code needs haskell98 because of the module System, and base because of Data.Monoid. Did I miss something?

@favonia
favonia commented Feb 18, 2013

OK. I added the new tests, built the benchmark but am still not sure how to do the benchmark.

@gregorycollins
Member

Start the server (make sure to use +RTS -A4M), get httperf, and run it against the server under test both before and after your patch.

Sample command lines here: https://github.com/snapframework/snap-benchmarks/blob/master/results.txt

@favonia
favonia commented Feb 19, 2013

I see. Will try to get to this this weekend.

@favonia
favonia commented Feb 24, 2013

Hi, I ran a simple benchmark and the result is here. capture0 has a nested routing for the path /id/id/id/id/id and capture5 has a nested routing for /:id/:id/:id/:id/:id. vanilla denotes the library without this patch. In my naive interpretation, there's no obvious slowdown because of the patch. Also, to my surprise, while compiled against the patched library, capture0 seems slower than capture5. I'd like to hear your opinions on these numbers.

@paluh
paluh commented Dec 11, 2014

Hi,
I want to upvote this feature. What is your final decision about it and this pull request?

@favonia
favonia commented May 6, 2016

@paluh My PR needs updates but I am occupied by other things now. It would be great if you could do it, and maybe that would accelerate the merging. As far as I remember, both snap-core and snap-server need to be patched at the same time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment