Tcl Source Code

View Ticket
Login
Ticket UUID: 93eb73784ae4c3b2285133a98df8163144eba291
Title: zipfs interaction with tcl_findLibrary causes slow loading
Type: Bug Version: 8.7
Submitter: juliannoble2 Created on: 2023-09-05 20:12:12
Subsystem: 37. File System Assigned To: nobody
Priority: 5 Medium Severity: Important
Status: Open Last Modified: 2023-10-08 03:48:40
Resolution: None Closed By: nobody
    Closed on:
Description:
Loading Tk on windows (non-tclkit) can be excruciatingly slow. e.g 5+ seconds 
The slow loading occurs within tcl_findLibrary

(related but different code path - if the tcl8.7 library files aren't installed - Tcl_Init seems to take a long time before eventually loading successfully from tcl87.dll)

This appears to be a combination of bugs/isues. 

1) zipfs exists $filename  always returns 0 when prefixed with the ZIPFS_VOLUME "//zipfs:/" but can be used if the prefix is dropped e.g

    % zipfs mount
    //zipfs:/lib/tk C:/tcl87/bin/tk87.dll
    % zipfs list //zipfs:/lib/tk/tk_library/tk.*
    //zipfs:/lib/tk/tk_library/tk.tcl
    % zipfs exists //zipfs:/lib/tk/tk_library/tk.tcl
    0
    % zipfs exists /lib/tk/tk_library/tk.tcl
    1

I notice the androwish zipfs implementation (which is diverging in interesting ways as noted in an earlier ticket) has fixed this so that it is consistent with for example 'zipfs info' which requires the prefix. 

2) tcl_findLibrary is using zipfs exists with the prefix so always returns zero.
This means the zipfs is mounted ok - but:

  mount is called twice with different capitalisations of driveletter

  catch {zipfs unmount $archive}  is called after the first mount - which does nothing because $archive is the wrong argument here - it should be the mountpoint not the name of the zip file 

(no-args error message from zipfs unmount is misleading in this regard)

    The second mount will fail because the first one is still in place - but it's wrapped in a big catch so things go on.


3) bad interaction with tcl's 'file exists'
tcl_findLibrary now has a list of paths to check - some of which begin with //zipfs:/  others from the external filesystem

  Presumably because //zipfs:/  looks roughly like a UNC path:

  file exists //zipfs:/somewhere

  will be very slow - on the order of seconds, when the path doesn't exist.
  
  (file exists on a //zipfs:/ path which is mounted and exists is fine)

This problem with file exists - whilst at the core of the delay - could be worked around if the 'zipfs exists' issue was fixed as this could be used for fast testing of those paths.

Once //zipfs:/ paths are mounted - they perform well on windows (faster than reading equivalent files from installed files)

There is an inconsistency between tcl with library files resulting in no //zipfs:/ mount

and Tk with library files installed results in them being accessed from the //zipfs:/ mount

In summary:
Tcl loading is slow without libs installed - Tk loading is slow whether they're present or not.

Given the performance boost from //zipfs:/ mounts - it may make sense to always load from the dlls in tclsh/wish when available even if user has installed the libs - as presumably they can use the relevant environment var to force loading from the filesystem if required (untested).
User Comments: apnadkarni added on 2023-10-08 03:48:40:
Now zipfs claims all paths under //zipfs:/ irrespective of whether a file exists or not. Committed as [1a5b7fb09d].

apnadkarni added on 2023-10-05 15:38:55:

Now being more familiar with the zipfs source, I've concluded that the zipfs file system can "claim" all paths under the //zipfs:/ volume as its own even if the corresponding file does not actually exist. This is analogous to how paths like C:/x/y/z are treated as under drive C: even though the file may not exist.

This will avoid non-existent paths being passed to Windows as "UNC".

Note this differs from androwish because there the mount points can be any arbitrary path and hence an existence check must be made to determine zipfs actually owns the path.

The above is implemented in branch apn-zipfs-pathclaim but I plan to merge it in the next day or two unless someone sees a problem with the change.


jan.nijtmans added on 2023-09-08 08:54:00:

> Unfortunately I don't know the source of $tclDefaultLibrary so I don't know the implications/possible side effects of this reordering.

$tclDefaultLibrary is only used on Windows and MacOS, because the runtime determination where the initialization scripts are differs there. I don't see any problem reordering.


jan.nijtmans added on 2023-09-08 08:52:01:

I merged the bug-93eb73784a branch to core-8-branch, with the exception of 2 parts which need further consideration:

1) The change to "zipfs exists", no longer auto-appending the zipfs prefix

2) The prefix change from //zipfs:/ to /zipfs:/ (contrary to the TIP)

All other suggested changes look good to me, and don't contradict the TIP.

Let's see how far we get with this


juliannoble2 added on 2023-09-07 19:21:46:
On review - while timings are slightly better for zipfs mounts - I think the bulk of the performance difference I'm seeing is from the fixes avoiding other unneccessary filesystem calls/mount attempts etc.

The patch below improves things slightly in that it allows the zipfs to be used even if  lib/tcl8.7 exists (but still allows TCL_LIBRARY to override)

Unfortunately I don't know the  source of $tclDefaultLibrary so I don't know the implications/possible side effects of this reordering.



Index: generic/tclInterp.c
==================================================================
--- generic/tclInterp.c
+++ generic/tclInterp.c
@@ -400,16 +400,16 @@
 "          lappend scripts {\n"
 "if {[regexp ^tcl(.*)$ [file tail $env(TCL_LIBRARY)] -> tail] == 0} continue\n"
 "if {$tail eq [info tclversion]} continue\n"
 "file join [file dirname $env(TCL_LIBRARY)] tcl[info tclversion]}\n"
 "      }\n"
+"      lappend scripts {::tcl::zipfs::tcl_library_init}\n"
 "      if {[info exists tclDefaultLibrary]} {\n"
 "          lappend scripts {set tclDefaultLibrary}\n"
 "      } else {\n"
 "          lappend scripts {::tcl::pkgconfig get scriptdir,runtime}\n"
 "      }\n"
-"      lappend scripts {::tcl::zipfs::tcl_library_init}\n"
 "      lappend scripts {\n"
 "set parentDir [file dirname [file dirname [info nameofexecutable]]]\n"
 "set grandParentDir [file dirname $parentDir]\n"
 "file join $parentDir lib tcl[info tclversion]} \\\n"
 "      {file join $grandParentDir lib tcl[info tclversion]} \\\n"

chw added on 2023-09-07 18:01:58:
Julian, the memfd_create/fdlopen should be avail on >80% of contemporary
POSIX platforms (I think MacOS has something similar), thus the mamooth
literally is Win32. Could be, that in the Firefox/Chrome universe there's
some homegrown DLL loader allowing alternate approaches, e.g. having the
DLL in a memory buffer filled over a super secure network pipe. Even the
good ole X11 XFree86 server has/had an ELF module loader, AFAIR.

I agree, could quickly get a big project. Further research needed.

Consequently, a futuristic Tcl load command might support the options
-chan with an argument of an open, readable channel and -data with a
bytearray argument for symmetry.

All makes sense in the age of huge address spaces, plenty of RAM and
high network bandwidth.

juliannoble2 added on 2023-09-07 16:57:33:
So far these fixes have made a big difference on windows, as it was heavily dependent on whether libs were installed. I remember testing teclab distribution a while back wondering why it was slow to start and the packager essentially gave a 'works for me'.. so fixing it could make a difference on the perceptions of usability of Tcl.

There is also a minor but nice performance boost when libraries are loaded from zipfs - there aren't many modules in there - but a couple of things like clock and encodings are helped along a touch too.
e.g 

    % time {exec printf {puts [clock format [clock seconds]]} | tclsh87}
goes from around 300ms to 200ms
    (first call to clock format is much faster)

This can start to make little tcl utility scripts better on windows.

Interesting to see CHW's comments re glob performance.
My testing so far shows glob performance in the zip mount to be way faster than windows already - although I believe that is because there are some other performance problems with windows filesystem access as ticketed at:
https://core.tcl-lang.org/tcl/info/411f52ed87e313dd

If there are further improvements to be had in glob for zipfs mounts then proportionally they may give better bang for buck on other platforms - but it still sounds like a win worth chasing.

Would love to see the memfd_create/fdlopen stuff ported - though getting that sorted on all platforms sounds like a big project.

chw added on 2023-09-07 15:27:55:
Remember that the zipfs.c of AndroWish predates TIP#430 by a few years.
And it's the crucial part of bootstrapping Tcl on the droid platform,
i.e. out of its installation (APK) package which already is a ZIP.

I believe that up until check-in [21ada87335] I more or less tried to
follow the development of tclZipfs.c but after it wasn't able due to
the huuuge number of changes.

Nevertheless, the ZipDirEntry/zipHash approach should please definitely
be ported since it will tremendously improve filename globbing. The
memfd_create/fdlopen piece would be nice to have but certainly isn't
that esssential.

apnadkarni added on 2023-09-07 15:18:46:

Jan,

Given the number of changes, I feel a TIP will eventually be required :-( In particular, I have no history of the basis on which the prefix //zipfs:/ was chosen as opposed to a URI scheme, the original [info nameofe] scheme or other options. You might have the background but I suspect others will have strong opinions on this as well.

/Ashok


jan.nijtmans added on 2023-09-07 14:54:23:

How's this? Please review/test


jan.nijtmans added on 2023-09-07 12:57:06:

> consider that zipfs mount takes it's arguments backwards compared to unix systems, androwishes' fork of zipfs and more importantly - Tcl's own vfs package

Indeed, that's bad! This change was done in [1f15130a9637c276|this] commit, but it doesn't match the documentation or the TIP #430 text.

> It may be worth considering reversing the behaviour of the zipfs mount command if it's not too late.

I don't think it's too late, since the actual implementation doesn't match the documentation. We should either adapt the documentation or the implementation.


juliannoble2 added on 2023-09-07 12:21:58:
Re Experiment: What if we changed "//zipfs:/" to "/zipfs:/"
branch bug-93eb73784a

Tested on windows - it solves the problem of slow tclsh startup when lib/tcl8.7 not present.

I also tested after doing make install-libraries - and it defaults to using the disk files - with no zipfs mount.  I think it should probably default to use the zipfs version when available and only use the files if pointed to by env(TCL_LIBRARY) or no zip archive exists but that's another issue I guess.

Testing with loading tk as a library and by running wish - this also solves the loading speed problem. 

    tcl_findLibrary is still calling mount twice - (which doesn't seem to hurt anything but shows zipfs exists is broken)
    zipfs mount /zipfs:/lib/tk C:/tcl/bin/tk87.dll

While you're at the documentation - consider that zipfs mount takes it's arguments backwards compared to unix systems, androwishes' fork of zipfs and more importantly - Tcl's own vfs package

It may be worth considering reversing the behaviour of the zipfs mount command if it's not too late.

zipfs unmount takes the mountpoint which, although different to androidwish which unmounts by taking the archive as an argument; is I think better - as it allows mounting/unmounting of the same file mounted at different paths.

Speaking of which... I really think consideration should be given to allowing zipfs to be able to mount files anywhere.

I think I understand the need for a special volume for bootstrapping issues and when no FS available - so it's ok as a default - but I don't see why zipfs shouldn't be able to mount zipfiles anywhere as well once a filesystem is up and running. 

Otherwise - we can resort to using the vfs::zip tcl-based system - but it seems like an odd requirement if zipfs is builtin - and is especially weird when the aforementioned mount arguments are reversed.

jan.nijtmans added on 2023-09-07 11:55:58:

Something which can be done easily: [e7567c469bc1ee2a]

This changes "//zipfs:/" to "/zipfs:/", which doesn't look like an UNC path any more.

That said, I like the changes in AndroWish, those should be ported to Tcl 8.7. Unfortunately, the Tcl 8.7 implementation also made improvements, I have no idea which of them were ported to AndroWish. The status is fully unclear to me.


chw added on 2023-09-06 20:43:00:
Another creative deviation w.r.t. TIP#430 in AndroWish is minimalistically
described in the last paragraph on

  https://www.androwish.org/home/wiki?name=ZIP+virtual+file+system

and the last sentence describes the IMO required step to finally get
decent glob performance. Admittedly this came after TIP#430 was out.

And for the record: I'm still unhappy with the slash/slashery and
colons and so on. We invented volumes for platforms which were
deliberately designed decades ago to be without.

juliannoble2 added on 2023-09-06 19:04:07:
Tk slow loading was fixed with another hack - this one in auto.tcl's tcl_findLibrary

Again - I only put this here as a proof of concept for narrowing down the real issue.

<code>
-	if {[interp issafe] || [file exists $file]} {
+    if {[string match "//zipfs:/*" $i]} {
+        set file_relative [string range $file [string length "//zipfs:"] end]
+        set test_exists_file [list zipfs exists $file_relative]
+    } else {
+        set test_exists_file [list file exists $file]
+    }
+
+
+	if {[interp issafe] || [{*}$test_exists_file]} {
</code>



It appears that other paths such as "zipfs:/"  have been used for windows in some places (e.g in practcl.tcl)

I'm not a fan of unnecessary platform differences - so I'm hoping the same ZIPFS_VOLUME can be used on windows as unix-like platforms.


I'm assuming there is some reason that a URI-like prefix like "zipfs://" isn't workable cross platform - but I haven't found that discussion.

juliannoble2 added on 2023-09-06 18:49:14:
The Tcl slow loading when no tcl8.7 libraries installed was fixed with the below hack in tclZipfs.c

Function: TclZipfs_TclLibrary

    -    found = Tcl_FSAccess(vfsInitScript, F_OK);
    +    found = ZipFSAccessProc(vfsInitScript, F_OK);

I'm not saying this is a proper fix - but I'm putting it here in case it assists.

juliannoble2 added on 2023-09-05 20:21:54:
Regarding poor performance of 'file exists' for nonexistant //zipfs:/ paths:

This is not a problem within the area of the mount e.g //zipfs:/lib/tk 


    % time {file exists //zipfs:/lib/tk/tk_library/nonexistant}
    4 microseconds per iteration
    % time {file exists //zipfs:/lib/NOWHERE/tk_library/nonexistant}
    6865814 microseconds per iteration
    % time {file exists //blah:/etc}
    6871529 microseconds per iteration

It is only a problem if the tested path falls entirely outside a mount.

(which happens during the tcl_findLibrary testing of dirs which first looks for //zipfs:/app/tk_library)

Attachments: