windows hash_map failure [Re: Proposal: restat rules]
Evan Martin
> On Sep 14, 7:50 pm, Evan Martin <mart...@danga.com> wrote:
>> On Wed, Sep 14, 2011 at 9:52 AM, Frances <frances.buonte...@gmail.com> wrote:
>> > BTW - I can't get the restat branch to build out of the box using
>> > VS2005.
>> > I believe the hash_map template parameters differ between gcc an VC
>> > Seehttp://www.gamedev.net/topic/405995-hash_map-header/
>>
>> Can you paste the error message?
>
> The essence is
> 2>C:\Program Files\Microsoft Visual Studio 8\VC\include\xhash(127) :
> error C2039: 'bucket_size' : is not a member of 'StatCache::KeyHash'
> 2> c:\src\ninja_stat\patched\src\stat_cache.h(47) : see
> declaration of 'StatCache::KeyHash'
>
> 2>C:\Program Files\Microsoft Visual Studio 8\VC\include\xhash(128) :
> error C2039: 'min_buckets' : is not a member of 'StatCache::KeyHash'
> 2> c:\src\ninja_stat\patched\src\stat_cache.h(47) : see
> declaration of 'StatCache::KeyHash'
>
>
> 2>c:\src\ninja_stat\patched\src\stat_cache.h(51) : error C2976:
> 'stdext::hash_map' : too few template arguments
> 2> C:\Program Files\Microsoft Visual Studio 8\VC\include
> \hash_map(89) : see declaration of 'stdext::hash_map'
>
> MS uses the third (defaulted) template param for hash_map as a policy
> for both the key and the comparison.
Ah, then this was probably me:
https://github.com/martine/ninja/commit/686fd98828e27008ddd0f58b4c180f536168d49c
https://github.com/martine/ninja/commit/3fbb25b2cc66f236a39b728d20e5d44da23612ac
It should be easy to adjust the ifdefs in hash_map.h to do the right
thing. I don't have a Windows box handy so please send a patch if
you've got one. :)
Frances
On Sep 15, 3:54 pm, Evan Martin <mart...@danga.com> wrote:
> > On Sep 14, 7:50 pm, Evan Martin <mart...@danga.com> wrote:
> >> On Wed, Sep 14, 2011 at 9:52 AM, Frances <frances.buonte...@gmail.com> wrote:
> >> > BTW - I can't get the restat branch to build out of the box using
> >> > VS2005.
> >> > I believe the hash_map template parameters differ between gcc an VC
> >> > Seehttp://www.gamedev.net/topic/405995-hash_map-header/
>
> >> Can you paste the error message?
>
> > The essence is
> > 2>C:\Program Files\Microsoft Visual Studio 8\VC\include\xhash(127) :
> > error C2039: 'bucket_size' : is not a member of 'StatCache::KeyHash'
> > 2> c:\src\ninja_stat\patched\src\stat_cache.h(47) : see
> > declaration of 'StatCache::KeyHash'
>
> > 2>C:\Program Files\Microsoft Visual Studio 8\VC\include\xhash(128) :
> > error C2039: 'min_buckets' : is not a member of 'StatCache::KeyHash'
> > 2> c:\src\ninja_stat\patched\src\stat_cache.h(47) : see
> > declaration of 'StatCache::KeyHash'
>
> > 2>c:\src\ninja_stat\patched\src\stat_cache.h(51) : error C2976:
> > 'stdext::hash_map' : too few template arguments
> > 2> C:\Program Files\Microsoft Visual Studio 8\VC\include
> > \hash_map(89) : see declaration of 'stdext::hash_map'
>
> > MS uses the third (defaulted) template param for hash_map as a policy
> > for both the key and the comparison.
>
> It should be easy to adjust the ifdefs in hash_map.h to do the right
> thing. I don't have a Windows box handy so please send a patch if
>
> - Show quoted text -
Hi Evan,
I've started looking at making a patch. Up to now, I just reverted
your changes inside an ifdef _MSC_VER, to stick with a
hash_map<std::String, FileStat*>.
I'm trying to do it properly now. I presume this change to use the
external hash and comparison is an optmisation. Am I correct? Can you
suggest a good way of testing what I do? I presume you had something
specific in mind when you did this.
Cheers,
Fran.
Evan Martin
>> It should be easy to adjust the ifdefs in hash_map.h to do the right
>> thing. I don't have a Windows box handy so please send a patch if
>> you've got one. :)
>
>
> I've started looking at making a patch. Up to now, I just reverted
> your changes inside an ifdef _MSC_VER, to stick with a
> hash_map<std::String, FileStat*>.
>
> I'm trying to do it properly now. I presume this change to use the
> external hash and comparison is an optmisation. Am I correct? Can you
> suggest a good way of testing what I do? I presume you had something
> specific in mind when you did this.
Sure, the patches I introduced it in are these:
1) Make the map of path string -> path up-to-date state use char*
https://github.com/martine/ninja/commit/686fd98828e27008ddd0f58b4c180f536168d49c
2) Make the map of path string -> logged info about path use char* as well
https://github.com/martine/ninja/commit/3fbb25b2cc66f236a39b728d20e5d44da23612ac
In total it looks like they shaved 50ms off of Chrome no-op build times.
Unfortunately I don't have a useful test to prove that with, beyond
what I did at the time: build Chrome both before and after the patch.
Since the change is only an optimization and not a semantics change, I
think if the test suite passes you could consider any implementation
correct enough. I've never profiled Ninja on Windows, so there could
easily be some low-hanging fruit that is particular to MSVC.
Additionally, ExternalStringHash<Foo*>::Type stuff (see the second
patch above) is a bit of C++ hackery that is above my C++ skill level,
so if there's a better way to do that I welcome ideas.
Nicolas Desprès
Hi guys,
I am not a C++ expert too but these two patches above looks overkill
to me. If there is really such a difference between hash_map
implementation across compilers (as the link to the gamedev forum from
Frances says), then I am sure some cross-platform libraries /
executable have already found portable work around. I think it worth
to have a look at them to take a decision. Maybe something already
exists in the wild and we could just reuse it. I will try to find the
time to have a deeper look at this issue.
Cheers,
--
Nicolas Desprès