Optimize IMPORT_NAME

### django ###
Min: 0.823356 -> 0.679505: 1.2117x faster
Avg: 0.826658 -> 0.684047: 1.2085x faster
Significant (t=138.673808)
Stddev: 0.00710 -> 0.00744: 1.0472x larger
Timeline: http://tinyurl.com/ybzne8a

Other benchmark changes are insignificant. All tests pass.

Committed as http://code.google.com/p/unladen-swallow/source/detail?r=1123.

[Collin Winter, 2010-02-19 20:34:26]

PTAL

[Reid Kleckner, 2010-02-19 23:34:45]

http://codereview.appspot.com/217052/diff/14/17
File Python/llvm_compile.cc (left):

http://codereview.appspot.com/217052/diff/14/17#oldcode193
Python/llvm_compile.cc:193: // being watching, neither is being watched.
Was this deleted intentionally?

http://codereview.appspot.com/217052/diff/14/18
File Python/llvm_fbuilder.cc (right):

http://codereview.appspot.com/217052/diff/14/18#newcode2482
Python/llvm_fbuilder.cc:2482: this->Pop();
Don't these need to be decref'd also?

http://codereview.appspot.com/217052/diff/14/18#newcode2490
Python/llvm_fbuilder.cc:2490: LlvmFunctionBuilder::IMPORT_NAME()
I prefer the convention I adopted for LOAD/STORE_ATTR_fast where the fast path
does all the checks it needs and returns false if it can't do the optimization. 
Then the top-level version calls the safe code version.

[Collin Winter, 2010-02-22 21:35:22]

PTAL

http://codereview.appspot.com/217052/diff/14/17
File Python/llvm_compile.cc (left):

http://codereview.appspot.com/217052/diff/14/17#oldcode193
Python/llvm_compile.cc:193: // being watching, neither is being watched.
On 2010/02/19 23:34:45, Reid Kleckner wrote:
> Was this deleted intentionally?

Yes; this is no longer true. The asserts have been moved into the optimizations.

http://codereview.appspot.com/217052/diff/14/18
File Python/llvm_fbuilder.cc (right):

http://codereview.appspot.com/217052/diff/14/18#newcode2482
Python/llvm_fbuilder.cc:2482: this->Pop();
On 2010/02/19 23:34:45, Reid Kleckner wrote:
> Don't these need to be decref'd also?

Good catch. Done.

http://codereview.appspot.com/217052/diff/14/18#newcode2490
Python/llvm_fbuilder.cc:2490: LlvmFunctionBuilder::IMPORT_NAME()
On 2010/02/19 23:34:45, Reid Kleckner wrote:
> I prefer the convention I adopted for LOAD/STORE_ATTR_fast where the fast path
> does all the checks it needs and returns false if it can't do the
optimization. 
> Then the top-level version calls the safe code version.

I agree. I've converted this over to that pattern.

[Collin Winter, 2010-03-03 02:21:09]

Updated to r1120 baseline. Any other comments?

[Jeffrey Yasskin, 2010-03-08 22:32:39]

LGTM, aside from the comments below.

http://codereview.appspot.com/217052/diff/3001/4004
File Python/llvm_fbuilder.cc (right):

http://codereview.appspot.com/217052/diff/3001/4004#newcode232
Python/llvm_fbuilder.cc:232: #define IMPORT_NAME_STATS(field)
import_name_stats->field++
The name of this macro doesn't mention that it's incrementing something. :(

http://codereview.appspot.com/217052/diff/3001/4004#newcode2568
Python/llvm_fbuilder.cc:2568: // We should already be watching this dict, thanks
to the LOAD_GLOBAL
Is this going to be fragile as we add more watching? Or is there some logical
reason that WATCHING_BUILTINS always comes along with WATCHING_SYS_MODULES?

http://codereview.appspot.com/217052/diff/3001/4005
File Python/llvm_fbuilder.h (right):

http://codereview.appspot.com/217052/diff/3001/4005#newcode718
Python/llvm_fbuilder.h:718: llvm::SparseBitVector<NUM_WATCHING_REASONS>
uses_watched_dicts_;
I think you actually want a std::bitset
(http://www.sgi.com/tech/stl/bitset.html). SparseBitVector has lots of overhead
for storing only 3 bits and is dynamically-sized. BitVector has little overhead
but is still dynamically-sized. std::bitset is statically-sized.

[Collin Winter, 2010-03-09 00:18:11]

Committing, thanks.

http://codereview.appspot.com/217052/diff/3001/4004
File Python/llvm_fbuilder.cc (right):

http://codereview.appspot.com/217052/diff/3001/4004#newcode232
Python/llvm_fbuilder.cc:232: #define IMPORT_NAME_STATS(field)
import_name_stats->field++
On 2010/03/08 22:32:40, Jeffrey Yasskin wrote:
> The name of this macro doesn't mention that it's incrementing something. :(

Done.

http://codereview.appspot.com/217052/diff/3001/4004#newcode2568
Python/llvm_fbuilder.cc:2568: // We should already be watching this dict, thanks
to the LOAD_GLOBAL
On 2010/03/08 22:32:40, Jeffrey Yasskin wrote:
> Is this going to be fragile as we add more watching? Or is there some logical
> reason that WATCHING_BUILTINS always comes along with WATCHING_SYS_MODULES?

Commented better.

http://codereview.appspot.com/217052/diff/3001/4005
File Python/llvm_fbuilder.h (right):

http://codereview.appspot.com/217052/diff/3001/4005#newcode718
Python/llvm_fbuilder.h:718: llvm::SparseBitVector<NUM_WATCHING_REASONS>
uses_watched_dicts_;
On 2010/03/08 22:32:40, Jeffrey Yasskin wrote:
> I think you actually want a std::bitset
> (http://www.sgi.com/tech/stl/bitset.html). SparseBitVector has lots of
overhead
> for storing only 3 bits and is dynamically-sized. BitVector has little
overhead
> but is still dynamically-sized. std::bitset is statically-sized.

Done.