Tcl Source Code

tclPkgUnknown may check directories multiple 
times and uses 'file readable' more than is 
necessary.  This patch fixes these problems and 
provides a modest performance improvement for 
Tcl 8.4.2.

(relevant given the complaints in comp.lang.tcl 
about filesystem and package require performance).

Logged In: YES 
user_id=32170

Ok, applying the equivalent to the MacOS and MacOSX procedures.

Logged In: YES 
user_id=32170

I'll take a look at the mac side of things.  Regarding 
removing 'file readable', I don't think there's any need. 
As long as at least 1 file/source/open operation is 
going to take place on a Tcl_Obj, adding others won't 
impact performance much at all (in fact we should be 
better than 8.3 at least in terms of the increment in 
time). What this patch manages is to avoid *any* file call 
under certain circumstances.  That saves a whole bunch 
of normalization and utf-native conversion.

I may not be able to get to the Mac side before 8.5a, but 
I'll try.

Logged In: YES 
user_id=80530

Vince, are you willing to 
look over the committed changes,
and apply similar enhancements
to [tcl::MacOSXPkgUnknown]
and [tcl::MacPkgUnknown] ?

Thanks!

File Deleted - 42930: 



File Added - 43073: 687906.patch

Logged In: YES 
user_id=80530

Here's the updated patch.

Committing it now, please continue
to review and test.

Leaving this report open as a reminder
that similar enhancments can also be
applied to the Mac-specific package
unknown procs.

Logged In: YES 
user_id=80530

As noted in the chat, dropping the
[file readable] tests would re-open
Tcl Bug 217824.  It's conceivable
that the improved performance is
worth ungraceful reaction to file
permission problems that should not
happen, but I'll save that judgment
for 8.5a.  I really think the big
performance improvements will have
to come from other approaches already
mentioned.

Will add comments and consider
alternative list equality algorithms
(with ne or eq, natch) and commit
soon.

Logged In: YES 
user_id=72656

Do not use string compare for ==/!= testing - it's need to 
return -1/0/1 makes it more costly.  Use ne ... this is 8.4 after 
all.

Logged In: YES 
user_id=68433

Looks good.

As discussed on the chat, you can save some stat() calls by
omitting both calls to [file readable $file], since
unreadable pkgIndex.tcl files are handled by the subsequent
"[catch {source $file} ...]", at the cost of a minor change
in semantics (unreadable pkgIndex files would log error
messages, which they do not at present.)

The test for list equality (~line 516) could use a comment;
it's not immediately obvious from the code what's going on.

Another way of testing for list equality just occurred to me:

# Check if $auto_path has changed:
set equal [expr {[llength $old_path] == [llength $auto_path]}]
if {$equal} {
    foreach e1 $old_path e2 $auto_path {
        if {[string compare $e1 $e2]} {
            set equal 0
            break
        }
    }
}

Anyway, back to DGP.  This looks comittable.

Logged In: YES 
user_id=80530

please give this a careful
check for inclusion in 8.4.2.

Logged In: YES 
user_id=32170

I prefer dgp's patch (never liked that if {$listone == 
$listtwo} stuff), which also seems to work, so please do 
check in!

Vince.

Logged In: YES 
user_id=80530


I'll agree to disagree on whether
the current behavior of
[tclPkgUnknown] represents
"correctness".  :)

Correct or not, it's been that way
long enough, and there's enough
stuff deployed depending on it, I
agree we can't change it now, and
need to "look forward" instead.

Both Vince's and my patch 
preserve current functioning.  They
just reorder the logic a bit to reduce
the number of [file] calls (which might
be expensive on some VFS's).   I'm 
OK with accepting either one for
8.4.2, if that meets Jeff's standards
for "code slush".  Preference, Vince?

More performance improvement will
have to come from a caching scheme,
or from alternative [package unknown]
callbacks.  Will file those as other reports.

Logged In: YES 
user_id=72656

tcllib messed with auto_path.  It is not good to throw away 
correctness for speed, so we should continue to just look 
forward on this one.

File Added - 42930: 687906.patch

Logged In: YES 
user_id=80530


Here's a new patch that tries
to avoid extending $use_path
with multiple copies of the
same directory, and tries to
avoid generating the string rep
of the $old_path and $auto_path
variables.

Can you check it for correctness,
and see if it improves performance
any more?

Still curious if the right answer 
isn't to chuck all the complexity
and revert to the [tclPkgUnknown]
of 8.3.0.  Well-behaved index scripts
shouldn't be messing with auto_path
anyway.

Logged In: YES 
user_id=32170

I did run some simple benchmarks on Windows (using a 
modified file.bench that Jeff now has). This speeded up 
the package startup on my machine by about 20%. The 
changes, as you can tell, are more to prevent 
nasty 'worst cases' from emerging.  In particular, if you 
have, say, [info library] in your auto_path multiple 
times, I imagine this patch could provide a very large 
improvement (but I haven't tested that carefully).

Logged In: YES 
user_id=80530

At first glance, this looks pretty good.

I'm curious if we have any benchmarks,
or can find someone to run some, which
will demonstrate that this patch makes
a measurable difference in those situations
where tclPkgUnknown performance is
a problem.

I'm also curious how the 8.4.1 version
and this patched version compare
with the [tclPkgUnknown] that was
part of Tcl 8.3.0 (Revision 1.11 of
file library/package.tcl).  Would be
nice to rollback those complications
that were arguably only added to
work around bugs in tcllib's package
indexing.

File Added - 42652: package.diff

Logged In: YES 
user_id=32170

attached patch (sf always fails the first time for me...)