Bazaar on IronPython

[Martin (gzlist)]

Thanks for the feedback. You and Andrew have gone down similar lines
on some of the topics, so I'll try and split up my responses across a
couple of different messages in a sane manner.

2009/6/29 Martin Pool <mbp at sourcefrog.net>:
> [bumped up]
> I haven't read your patch yet, let me know if you actually want a review.

I do suggest looking it over, makes the basis of some of the points
being discussed here clearer.

The one line changes and some of the comments are the more interesting
parts from a bazaar perspective, rather than the added blocks of code,
so it's not a lot to read.

>> Bazaar has a lot of platform switching code in various different
>> places. I get a strong impression that anything touching sys.platform
>> or errno should be migrated to osutils.
>
> I would probably agree with that, pending a look at the particular
> cases.  One other that comes to mind is that tests may also want to
> switch on the platform, but even there I'd say that in general tests
> should be depending on Feature classes present on the platform or not.
>
> We could eventually potentially add a test_source saying you must not
> look at sys.platform other than in particular places.

I think all the right ideas are there already, just inconsistently
applied. A rule of thumb, or even a test as you suggest, that serves
as a reminder to convert quick fixes into a more maintainable form
would be good.

>> Using ref-counted RAII is convenient. To change everything to
>> finally-close would be too much pain for no actual benefit.
>
> Well, I agree it's more convenient, but I don't think it's a good
> tradeoff.  The problems with running code from __del__ are: exceptions
> raised there are lost, you don't know when precisely it will run, it
> may not run at all before the process exits, because of the preceding
> points it's hard to test, it may impede garbage collection, and
> perhaps more.  The net effect is that you cannot use it for anything
> that really has to be done, like releasing a lock.  Just about the
> only thing we use it for at the moment is giving a warning that the
> object was not closed, and I think even that is questionable.  (See
> https://bugs.edge.launchpad.net/bzr/+bug/391024)

Yes, I've decided in the past after much trying that tying some
important resource to the lifetime of one of my python classes is
basically never a good idea. However, that's with the vagaries of
__del__ and related problems. For built-in objects like file which can
be cleaned up by the OS anyway, it's pretty much second nature to
leave python to do the right thing, and design interfaces that trust
that's going to happen. It's much like nix programmers just assuming
they can write code that deletes files they have open and so on. The
bug you link there is evidence of how common it is in the codebase.

My point was simply that, while everyone is using a python
implementation that *does* refcount, and removing that assumption
makes the code worse not better, it's not worth worrying over too
much.

> We do use zlib fairly extensively, so we do need something.  Perhaps
> we can add a layer that will work on DeflateStream.

This is an issue for the IronPython team I think. Unlike other more
obscure features that might justify a bazaar shim (like the third
version of the lock code for windows added in the patch), zlib is so
widely used in the python world, including the core, they need an
implementation.

The good news here is the author of the zlib emulation library got
back to me after I reported the problems I'd found and put out a new
release with the fixes the same day. So, there's a working option on
this front in the interim.

>> Likewise, it seems to make inspect_for_copy.py pointless. Is there any
>> ongoing auditing of perf hacks like the lazy imports and regexps and
>> this to ensure they remain net wins?
>
> Just the sort of thing you're doing here, people occasionally looking
> into the performance for things that look bad.  It seems like we
> should be able to write a script that makes assertions about what is
> loaded and then checks both that it is reasonable and that it's
> consistent with what's supposed to be lazy-loaded.  But that's not
> been done afaik.

Well, I guess there are the real-tasks timings that get run regularly,
and are strictly more useful than micro-benchmarks. The lazy import
mechanism in particular though is really powerful but seems
under-defined, I found it hard from looking at the tops of various
files to work out what the actual expected outcomes were.

>> Overall, it was quite impressive how little needed altering, though
>> the path to discovering those small changes was often winding, even
>> the root cause was the same as a previously handled issue.
>
> I think that's a good reason to get a mergeable patch merged, so that
> next time you or someone comes to it they won't be starting from
> scratch.

I did start this as a plan to clean up osutils, so some of what I've
discovered here should make its way into a less hacked-together form
that will hopefully make all platform-dependant code clearer, as well
as making this kind of 'porting' easier in future.

Martin