search

rails / journey

A router for rails
221 stars 57 forks source link

Formatter#generate: optimize parameters constraints code #38

Closed bogdan closed 11 years ago

bogdan commented 12 years ago

Tried to optimize a way how router handles constraints. During the process find out that constraints for optional parts doesn't work at all. They are simply ignored. This doesn't sound well. This patch changes that test to a different behavior: Constraints for optional parts are not ignored anymore. As @tenderlove said: journey test suite in most cases just tests journey internal and we can change behavior in case it doesn't break actionpack test suite.

Also this patch offers 10% performance boost for named routes generation. https://gist.github.com/2915825

Running benchmark with current working tree
Checkout HEAD^
Running benchmark with HEAD^
Checkout to previous HEAD again

                    user     system      total        real
--------------------------------------generate without name
After patch:    0.340000   0.010000   0.350000 (  0.349404)
Before patch:   0.320000   0.030000   0.350000 (  0.356459)
Improvement: 2%

-----------------------------------------generate with name
After patch:    0.040000   0.000000   0.040000 (  0.033736)
Before patch:   0.040000   0.000000   0.040000 (  0.037829)
Improvement: 11%

-------------------------------------generate with globbing
After patch:    0.330000   0.000000   0.330000 (  0.340345)
Before patch:   0.330000   0.020000   0.350000 (  0.350696)
Improvement: 3%
rafaelfranca commented 12 years ago

This pull request cannot be automatically merged.

bogdan commented 12 years ago

Rebased.

schneems commented 12 years ago

There hasn't been any discussion on this issue since it was rebased. I believe additional code was introduced into the formatter since. Is this implementation with the intent of speeding up the generation time under consideration? If so what should start the conversation, otherwise we should close the issue.

bogdan commented 12 years ago

@schneems don't see additional code here: Constraits verification code just moved from one place to another. This makes it more lazy: this code got executed only in cases where it is needed.

rafaelfranca commented 11 years ago

@bogdan journey was merged on Rails so I'm closing this one. Mind to open a pull request for the Rails repository if it is still relevant?

Sorry for the delay :bow:

bogdan commented 11 years ago

sure