Re: [PATCH] Remove APR ICONV dependency on Windows (was SVN Win32 Developers -- need some help)

On 7/1/07, Stefan Küng <tortoisesvn@gmail.com> wrote:
> Ivan Zhakov wrote:
> > On 7/1/07, steveking <tortoisesvn@gmail.com> wrote:
> >> Ivan Zhakov wrote:
> >>
> >> > Sorry, I was busy and I have not time to report my research results.
> >> >
> >> > My patch almost complete and I need somebody's eyes to look and
> >> > confirm that I'm on right way (or say that I'm on wrong way :)
> >> >
> >> > I used Windows MIME database to convert page name to Windows page
> >> > identifier.
> >> > And most important question is it acceptable to read Windows MIME
> >> > database directly from registry instead of using MLang API [1]? I
> >> > didn't use MLang API, because it requires
> >> > CoInitialize()/CoUninitialize().
> >>
> >> I think it would be better to use the API instead of reading the
> >> registry. Because the registry is not documented and could change any
> >> time.
> >> About CoInitialize(): we could call that in svn_utf_initialize() and
> >> call CoUninitialize in an apr cleanup handler.
> > Actually registry is documented, at least for Windows CE :)
> > I agree that using API is better, but it's not too easy. We have to
> > call CoInitalize() in each thread that using COM. Another problem that
> > we don't require user libraries to call svn_utf8_initalize().
> >
> > As workaround of problems above we can call to
> > CoInitialize()/CoUninitalize() every time in win32_open_xlate(). This
> > function get called only once for each page identifiers, so it
> > shouldn't cause performance degradation. What do you think about it?
>
> I forgot about the backward compatibility. Otherwise we could just
> document that CoInitialize() must be called for every thread.
>
> If the registry is documented, then we can of course use that. I don't
> think it will be faster than using the API since registry accesses are
> expensive too, but at least for most conversions it's not needed anyway.
>
> [snip]
> >> I know this is the correct way to do this, but I don't like it. You
> >> allocate memory here which is needed only for a few lines of code. That
> >> memory is allocated in the apr pool, which we don't know when it gets
> >> cleared again (maybe not until the app exits because it's the same pool
> >> that holds the translated strings). That means that a lot of memory is
> >> 'wasted'.
> >> For example, assume you do an
> >> svn log -v url/to/kde/repository/root
> >> and you'd have to keep that log information alive until your app exits.
> >> With using an apr pool to reserve memory here that would mean that a
> >> *lot* of memory is kept allocated but not used anymore.
> >> That's why I used calloc() and free() in my patch - which isn't really
> >> good either in terms of platform independence, but it won't consume a
> >> lot of memory for nothing. Question is: do we really need apr pools here
> >> for this Windows-specific code?
> >> The correct way here may be to create a subpool and destroy that subpool
> >> before the function exits. But that's an expensive operation.
> >>
> >> In case you're wondering why someone would do such a thing (get the log
> >> info for a whole repository and keep it until the app exits), the
> >> revision graph in TSVN needs to do exactly that.
> > Yes, I know about this problem. I propose to discuss it separately
> > after committing simplest implementation. Because we can made several
> > optimizations: for example try to allocate small buffers on stack.
>
> Yes, that would improve the performance a lot. Since most conversions
> are done on paths, I suggest to allocate buffers smaller than
> MAX_PATH*sizeof(WCHAR) on the stack, the rest could use an apr subpool.
>
Hi Stefan,

I've implemented allocation of small buffers on stack in r26357. Now
buffers smaller than MAX_PATH allocated on stack.

I'm not sure about temporary allocating buffer using apr
subpool/malloc, so I didn't implement that. Feel free to propose patch
and/or open this topic on list if you want.

-- 
Ivan Zhakov
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org