Storm

Merge lp://staging/~therve/storm/cache-compile into lp://staging/storm

cache-compile
Merge into trunk

Proposed by Thomas Herve on 2011-08-14

Status:

Merged

Merged at revision:

400

Proposed branch:

lp://staging/~therve/storm/cache-compile

Merge into:

lp://staging/storm

Diff against target:

282 lines (+40/-26)

4 files modified

storm/expr.py (+14/-5)
storm/info.py (+10/-11)
tests/expr.py (+7/-1)
tests/info.py (+9/-9)

To merge this branch:

bzr merge lp://staging/~therve/storm/cache-compile

Low

Fix Released

Link a bug report

Reviewer	Date Requested	Status
Gustavo Niemeyer	2011-08-14	Approve on 2011-09-07
Jamu Kakar (community)		Approve on 2011-09-07
Free Ekanayaka (community)		Approve on 2011-08-22
Review via email: mp+71473@code.staging.launchpad.net

Description of the change

Here it is. The branch adds a new attribute on Table and Column to cache the compilation. AFAIU, those compilations don't depend on the state, so we can store directly the value. It also changes the ClassInfo to store the table as a Table object instead of a SQLToken, to go through the compile_table logic.

I made a quick benchmark to show the gains:

| Trunk | Branch | Gain
10 inserts, 3 colums | 1.68s | 1.55s | 7.8%
100 insersts, 3 columns | 14.15s | 12.79s | 9.6%
20 insersts, 10 columns | 5.38s | 4.71s | 12.4%

I think it's worth it considering the low impact of the changes.

Revision history for this message

Gustavo Niemeyer (niemeyer) wrote on 2011-08-14:

Nice!

Would you mind to repeat the benchmark with 1000 inserts and 10 columns each, to see a bit better the effects? The first number feels a bit suspect (hard to believe 10 inserts would take 1.6 seconds).

review: Needs Information

Revision history for this message

Thomas Herve (therve) wrote on 2011-08-15:

Gustavo: sorry this was explicit, but it's run 1000 times over.

Revision history for this message

Thomas Herve (therve) wrote on 2011-08-15:

Here's the script I used, with the last run: http://pastebin.ubuntu.com/666307/

Revision history for this message

Free Ekanayaka (free.ekanayaka) wrote on 2011-08-22:

Hi Thomas,

that's indeed significantly faster for use cases similar to the one in the attached script.

[1]

- self.table = SQLToken(self.table)
+ self.table = Table(self.table)

I guess this is to make use of the cache? Are SQLToken and Table fully interchangeable in this case?

[2]

It'd be good to add a couple of unit tests for the new behavior.

review: Approve

lp://staging/~therve/storm/cache-compile updated on 2011-08-24

399. By Thomas Herve on 2011-08-24: Add tests for compile_cache value

Revision history for this message

Thomas Herve (therve) wrote on 2011-08-24:

[1] Yes, it's for using the cache. table has been documented as an expression, so I don't see any problem changing it. The compilation passes token=True which results to the same result.

[2] I added some tests.

Thanks!

Revision history for this message

Jamu Kakar (jkakar) wrote on 2011-09-07:

Looks good to me, +1!

review: Approve

Revision history for this message

Gustavo Niemeyer (niemeyer) wrote on 2011-09-07:

Thomas, I mentioned 1000 inserts, not running it 1000 times.

Either way, sounds like it's improved things in practice, so +1.

review: Approve

Revision history for this message