Hammerspoon / hammerspoon

Staggeringly powerful macOS desktop automation with Lua
http://www.hammerspoon.org
MIT License
11.94k stars 578 forks source link

Performance of loading docstrings #2145

Open cmsj opened 5 years ago

cmsj commented 5 years ago

I've noticed for a while that reloading HS causes a noticeable beachball and prompted by someone else asking on IRC, I finally profiled it.

I'm pretty sure that it's entirely down to hs.json.decode time, which, without a config, can only mean the docstrings being loaded (5.3MB of JSON).

@asmagill fancy spitballing some ideas of how we could optimise this?

asmagill commented 5 years ago

Without actually running any tests, I'm going to go out on a limb and say that I doubt the issue is how long it takes the macOS to parse the JSON data... it's more likely either the load time of the docs.json file (possible, but probably not the worst offender given modern drive and memory speeds) or the time it takes Lua/LuaSkin to convert the data into lua objects (tables, etc.) and build the metatables.

What the hs.doc module does is load the json file into a string, parse it using the macOS API, and then convert it to a table. This table is then iterated through to add metatables so we can do things like help.hs.screen.primaryScreen in the console to see the help directly.

I can run some tests this weekend to see about taking out that last step... IIRC we should still be able to do things like hs.doc.help("hs.screen.primaryScreen") possibly with some minor modifications but we'd lose the shortcut.

It may also be worth seeing about adding a function to hs.json that takes a path as a and returns the contents of the file... this would take out the first step which would certainly remove the need for Lua to allocate 5.3mb for a string, then collect it almost immediately... we'd still have the time and memory it takes to convert the objective-C object into a lua table, but it might help.

As a more drastic measure, we could move from using json to using sqlite (which could be created at compile time) and never actually load the whole help database into lua memory, just query as needed... I'd have to think about how we might be able to do something similar to the shortcut currently available (I use that shortcut OFTEN and would definitely miss it; I suspect I may be in the minority, though...) but that could come later

The sqlite approach would also impact loading additional help for third part modules or spoons at run time, but I think I may be the only one who really takes advantage of this. It should still be possible, but would take some thought...

asmagill commented 5 years ago

edit -- It may also be worth seeing about adding a function to hs.json that takes a path as an argument and returns the contents of the file as an already parsed table ...

Just to make it clear.

cmsj commented 5 years ago

looking at the Instruments time profiling, I think you're on the money with the pushNSObject angle. I'm not thrilled by the idea of doing an sqlite database, but I was wondering if rather than convert the 5.3MB of JSON into a proper Lua table, maybe we should simply keep it in C land and present it as a userdata with sufficient metatables to make it behave like a table?

latenitefilms commented 5 years ago

That's interesting!

As a temporary workaround in the meantime, I wonder if it's worth adding an option to disable the JSON registration in hs.doc, so that when you're coding (and doing lots of reloading) you can avoid this wait time?

I was also thinking if there's anyway to do some caching? For example, you could store the file sizes of all the JSON files and only read them if the file size has changed? You could save this cache as a standard Lua table (as opposed to JSON) to avoid another step as well?

cmsj commented 5 years ago

@latenitefilms anything being cached in Lua isn't going to help unfortunately, because we destroy the entire Lua interpreter on a config reload. We either make the loading process dramatically more efficient, or shift it to C where it can be cached across config reloads (and/or made dramatically more efficient :)

cmsj commented 5 years ago

for reference, on my 2017 12" MacBook (so a 5Watt CPU that is not at all good for doing complex stuff), the NSJSONSerialisation call takes 37ms, where the pushNSObject takes 3.9s šŸ™‚

asmagill commented 5 years ago

@latenitefilms you could create your own hs.doc and see what breaks.

In your hammerspoon config dir (usually ~/.hammerspoon) do something like this:

$ cd ~/.hammerspoon
$ mkdir -p hs/doc

Then create init.lua in ~/.hammerspoon/hs/doc so that it contains something along these lines:

local module = {}

module.registerJSONFile = function() end

return module

then you can "toggle" the full behavior vs "minimized" version by simply renaming the init.lua file to something else and reload hammerspoon.

latenitefilms commented 5 years ago

@cmsj - I was just thinking if you convert the JSON data to Lua (i.e when Hammerspoon first detects a new spoon, it creates a unique Lua file containing the JSON data of that spoon in a cache directory), then you can just use dofile() to load the contents of this cache, rather than reading the JSON file. So you'd still need to load the data each file Lua reloads - but it would be faster the second time. If the spoon has changed (for example), the file size would be different (this data would also be stored in the Lua file cache for the spoon), and you'd delete it and re-create the cache from the JSON file.

Having said that - doing this all in Objective-C land probably makes more sense.

asmagill commented 5 years ago

Keeping it all in Objective-C should be doable... and I think I can do it in a way that doesn't break anything, though to be honest, I think the only things that it should impact would be hs.doc itself and the web version accessible through hs.doc.hsdocs.

It will probably be this weekend before I can do much, but let me mull this over in the back of my mind for a bit.

asmagill commented 5 years ago

FWIW, I think I'll also add the ability to pass in a path to hs.json.decode anyways -- the two most common sources of JSON data are likely HTTP requests and files.

In the file case, there's no reason to have to add lua code just to open a file and read everything into a string that you're just going to pass to hs.json.decode and then let get collected.

cmsj commented 5 years ago

@asmagill agreed having the path makes sense.

On the JSON decoding, I'm wondering if we should just always have hs.json.decode return one of these fancy C pretend tables. We're probably the biggest user of it, but if people are fetching stuff from the web, they might appreciate more performance.

latenitefilms commented 5 years ago

Maybe rather than modifying hs.json.decode() and hs.json.encode() you should add hs.json.read(path) and hs.json.write(path)?

asmagill commented 5 years ago

@cmsj, functionally, that shouldn't be a problem. My only concern would be if the jsonObject is passed to another function where a table is expected, and the type is checked like we do with LuaSkin's checkArgs: or similar... we can make LuaSkin smart enough to know that it's a table in all functional ways, but we can't change what lua reports it as with type or with the c-api functions that access things bypassing the metamethods.

Adding an "asTable" method to the jsonObject is easy enough which reverts back to the way things are now, so the question we need to answer is do we make the table the default and the jsonObject something you have to specifically request, which would preserve backwards compatibility; or vice-versa which should improve performance in almost all cases where a "real" table isn't needed but forces the user to know about "asTable"?

edited to note this is in response to cmsj's point -- this thread is hopping right now!

asmagill commented 5 years ago

@latenitefilms hadn't thought about writing, but the same logic applies, and I wan't planning to "replace" decode; it either would be a new function to read or an extension to the existing function (probably the former).

Should write fail if the file already exists? Would adding a flag as a second argument, which if true would replace the file if it already exists, be a sufficient "safeguard", or should we require the user to explicitly delete it with something like os.exec("rm ...") before invoking the write?

latenitefilms commented 5 years ago

@asmagill - Good question. I guess whatever we do, it should be consistent across all of Hammerspoon (i.e.hs.plist.write()). I think adding a replace variable to hs.json.write(path, replace), hs.plist.write(filepath, data, binary, replace) makes sense.

asmagill commented 5 years ago

After a quick glance at the json module, I have a couple of thoughts...

Don't have a time frame at the moment, but unless anyone has objections or other thoughts, I'll start this weekend and should have a better idea on how long it might take then...

In the mean time, is it worth adding a flag to hs.doc like @latenitefilms suggested above that bypasses the automatic loading the documentation (and adding spoons as they are loaded)? something like hs.doc.preloadAtStart([true/false]) which defaults to true (use hs.settings to get the value, if it doesn't exist, assume true). We'd have to document that online help will be disabled if this is set to false, see the web site for documentation, etc. Possibly even add a function to cause the loading of documentation later if the user decides along the way that they want to access it from the console but not have it preloaded by default. I'd like to confirm that it only disables help from the console and the built in help web-server, but assuming those are the only things affected, this should be a fairly simple modification to hs.doc and hs._coresetup.

asmagill commented 5 years ago

A further thought -- since the actual jsonObject is just a "Standard Foundation Object" as defined by Apple, I'll give some thought to making this part of LuaSkin so that other modules which may generate large chunks of data can take advantage of keeping things in Objective-C land, too... module developers would then have the option of passing data back and forth as Lua objects, as Objective-C ones, or both with hopefully only minor modification...

latenitefilms commented 5 years ago

Does this also affect the auto-complete functionality in the Console?

latenitefilms commented 5 years ago

Thinking about it further, we actually use JSON files to store a lot (around 2-3MB) of data in CommandPost. This might/probably is what you're already suggesting, but it would be awesome if we could read and write from JSON files with the same ease as hs.settings.

For example, I'm thinking something like this:

myJSONFile = hs.json.new("~/Desktop/data.json")
myJSONFile:value("result", {["data"]="hello"})
local hello = myJSONFile:value("result")[data]

FWIW, this is how we're currently reading and writing JSON:

https://github.com/CommandPost/CommandPost/blob/develop/src/extensions/cp/json/init.lua

cmsj commented 5 years ago

@latenitefilms I think we should tackle reading first, and if that is a success, think about writing later.

@asmagill I think I have a partial idea for a way we could do this such that Lua sees a table type, but that table is really just a lightweight proxy object to the underlying NSDictionary. It requires one assumption for maximum performance and minimum RAM usage, specifically, that the Lua code which requests the object, will hold a reference to it for the entire time it's reading child objects (specifically child tables) so we don't have to do any [NSDictionary copy] stuff when subtables are requested.

If we create a table and give it a __index metamethod, we can go fetch values from the NSDictionary at will and return them to Lua using pushNSObject as normal. That's nice and easy for numbers/strings/etc, but won't work for tables because we'd then pay the conversion price again. Instead, I think we'd want to return a Lua table with enough data for it to be a second proxy object pointing at some child dictionary in the main NSDictionary.

Does that make sense to you? I'm only about 80% sure I'm thinking about this properly, and the official docs for __index contain some slightly opaque suggestions that if you return a table, it will get searched for the requested key, rather than returning it as the value. I'll see if I can find some time today to play with this idea.

asmagill commented 5 years ago

@cmsj, if you added a reference to the parent table in the metatable of the child object, (e.g. __parent = {parent table}) I think the requirement to hold the parent in the users Lua code might not be necessary (still a good practice, but not a requirement)... the trick would be to make sure the extra reference gets cleared when the child object is collected.

Of course that only works if the node you're at is also a table... but if I'm traversing the results of a decode and have reached say a string node, either I have what I want and am now done, or I'm already holding the parent of the current node somewhere (possibly in a recursive function call) so I can backtrack and check other fields.. so I think that's ok.

Your idea is a little lighter weight than I had planned, but on the surface I think it might still be portable enough to be of use in other places as well... one in particular I have in mind is hs._asm.axuielement and for that I would definitely want a link back to the parent in any object that wasn't an end-node because it makes it easier to find related items.

asmagill commented 5 years ago

And it goes without saying, you'll want to also implement __newindex so any additions or changes get reflected in the proper Objective-C object... the table in lua representing our position in the tree wouldn't actually have key-value pairs in it -- it would have to use __index and __newindex to modify/access/create these in the oC obj as needed.

Probably still need a method that returns everything from the current point as a true lua table, like decode currently does, for cases where the result needs to be passed to something that might query it through methods that bypass the metatable, but 9 times out of 10 you wouldn't need this...

asmagill commented 5 years ago

@cmsj --

and the official docs for __index contain some slightly opaque suggestions that if you return a table, it will get searched for the requested key, rather than returning it as the value. I'll see if I can find some time today to play with this idea.

Where do you see this? I've returned tables with __index functions before without a problem, so I'm curious about what they might be referring to.

latenitefilms commented 5 years ago

Thinking about this further... is there any reason why you need to load all these JSON's on startup anyway? Could you just load them if/when they're needed (i.e. someone accesses the help functionality)?

asmagill commented 5 years ago

@cmsj, as a first pass, here is a poorly written outline of what I see being necessary... it's a mix of psuedocode, lua, and objc, but I think it captures the essence of what we need... thoughts?

This would be easier in lua itself, converting it to strictly C will take some thought and probably trial-and-error, but as psuedo-code, I see somthing like this:

__internalObjects = table with weak-keys to store table for objC objects we've passed on to lua
    stored as {} = userdata representing actual object
    table in lua-space so when {} goes out of scope, being a weak key, so does userdata, invoking __gc on userdata to release object

__metatable = metatable attached to table returned
    __index(self, key) = lookup in __internalObjects with key `self` to get object;
                        if key == "asTable" then -- name of method can be changed, but whatever it is, we lose it as a possible key in the table
                            return [LuaSkin pushNSObject:obj]
                        else if object[key]  -- remember key can be an integer for arrays, so appropriate logic to check
                            return makeSpecialObject(object[key]) -- this checks if object is not array or dictionary
                        else 
                            return nil

    __newindex(self, key, value) =  lookup in __internalObjects with key `self` to get object;
                                    if object dictionary,
                                        object[key] = value
                                    else if object array then
                                        if key integer,
                                            object[key] = value -- how to handle assignment that makes array sparse?
                                        else if value != nil
                                            convert object to dictionary
                                            object[key] = value
                                    else -- shouldn't happen
                                        error?

    __pairs = not sure yet, but hs.canvas does something similar (but in reverse) so will review and decide then

    __eq(self, item) =  if item not table, return false
                        lookup in __internalObjects with key `self` to get object;
                        lookup in __internalObjects with key `item` to get object;
                        return object1 == object2

    maybe others, not sure yet...

makeSpecialObject(NSObject *obj)    
    if typeof(obj) is array or dictionary then:
        lua_newtable(L) ;
        assign __metatable to table on stack

        Create userdata for object to increase retain count for ARC; __gc will release
            does the userdata require other methods? not sure yet...

       // forgot this important line:
        __internalObjects[table-on-stack] = userdata

        return table on stack
    else
        [LuaSkin pushNSObject:obj]

edited to add the following additional thoughts

It goes without saying that if the initial data going into makeSpecialObject is an array or dictionary, we need to copy it, recursively, into a mutable form; otherwise __newindex will bomb... should we make a "read-only" flag an option?

I think the current hs.doc functions add a bunch of metamethods to the tables it receives from hs.json.decode... this approach would have to change because I don't think it's possible to make a "psuedo" table like we're proposing also support arbitrary metamethods on itself or it's members... I'll give it some thought, though...

further edited to add assignment to __internalObjects in psuedo-code; without that, retention goes out the window, even if they do save the original table!

further edit with further thoughts

If this is to be something we might make more generic and use in other places, then yes, a "readOnly" or "asIs" option needs to exist (and maybe be the default)... modifying an existing structure makes sense for a block of data read in from a file and converted from json into tables, but not for something representing, for example, a reading of something from IOKit.

It actually may be more efficient to keep the object as it is, the in __newindex, copy into mutable (if not already), make change, then copy back (if it wasn't mutable to begin with) to a non-mutable version and replacing the original. Would have to run tests, but it really only becomes an issue if we want the ability to change these large data sets without fully importing them into Lua... I like the idea, but maybe not for the first pass.

More as I think of them!

asmagill commented 5 years ago

@latenitefilms aside from the fact that hs.doc.help, hs.help, help, hs.loadSpoon, and the whole of hs.doc.hsdocs assume it's already done, probably not... have to give some thought on how to make sure triggering any of them (except hs.loadSpoon, other then maybe capturing a list of spoons to load with the docs, should the docs be loaded later) started the process....

I guess we have some options and we need to decide which are v1 tasks and which are v2 tasks because I think all of them have merit in different ways.

Have I missed anything?

latenitefilms commented 5 years ago

I kinda like the idea that if your init.lua config is empty, that Hammerspoon won't really load anything (apart from the bare essentials in _coresetup) - and as a result reloads will be really quick. I've personally never used hs.doc.help, hs.help or help - I generally just have a seperate browser window open to the API documentation if I need to check things.

I do however use auto-complete in the console all the time, however a quick glance at _coresetup reveals that it doesn't use these JSON files anyway.

I can't imagine it would be that difficult to only load this help data if/when required. As this is all in Lua-land, this is probably something even I can attempt. I think I'd prefer this, than having a setting to disable the automatic loading of the doc strings.

Improving JSON performance for reading & writing would be a MASSIVE benefit to the stuff I'm doing in CommandPost, so I'd love to see this sooner rather than later regardless of what happens with this help documentation stuff.

Unrelated, I might experiment myself with using SQLite and/or plain Lua (using dofile()) in place of the stuff we're currently saving to JSON files to see if there's any speed boost.

cmsj commented 5 years ago

I have some fairly hacky code that half works, but I couldnā€™t figure out exactly what the pairs and/or next metamethods need to do. The docs are scant and most examples are either for old Lua versions or donā€™t make any sense to me šŸ˜ Iā€™ll push a branch up shortly.

cmsj commented 5 years ago

https://github.com/Hammerspoon/hammerspoon/commit/5d7721de1d0e782ed985948786ae1820ecd16a69 is what I have so far.

chdiza commented 5 years ago

I'm strongly in favor of at least having some kind of setting I can put at the top of my init.lua to disable the slow stuff :)

I've been seeing these beachballs and otherwise laggy startups for years (including every Aqua login, since HS is among my login items). A snappy startup would be a huge boon.

asmagill commented 5 years ago

hs.doc is an amalgam of patches and extensions made to the doc expander that started way back in the Hydra days... I'm wondering if it isn't just time to scrap the whole things and start over from scratch, moving a lot of it into Objective-C entirely...

It's only referenced within itself, _coresetup, the httpserver documentation (which just shows how to start the built in web server for the documentation, it doesn't actually use the module) and maybe a few readme's talking about creating spoon documentation...

I'm thinking now that we put off the hs.json rewrite (except maybe for adding paths to read/write functions) and I just rewrite hs.doc entirely. As long as I retain the same public facing functions, I think the only things that will break (initially) will be hs.doc.hsdocs (the web server) and the ability to type help.hs.module.whatever into the console, both of which could be adjust to the new scheme with relatively minor (I hope) changes (they both currently make assumptions about the internals of hs.doc which will be changing).

latenitefilms commented 5 years ago

Sounds like a good plan to me!

I'm also thinking for my own JSON uses - I might actually get faster results using hs.plist - will have to do some testing to see.

asmagill commented 5 years ago

I'm hoping to have a trial ready sometime tomorrow -- it won't be fully functional, but I think I've managed to replicate most of the functions in Objective-C now, but I'm running into some issues parsing the names the way we want... NSRegularExpression is a royal pain to work with...

latenitefilms commented 5 years ago

@chdiza - In the meantime, if you do want to dramatically speed up Hammerspoon reloads, you can comment out the following:

Line 314-315 in _coresetup/init.lua

  hs.help = require("hs.doc")
  help = hs.help -- luacheck: ignore

Line 334-358 in _coresetup/init.lua

  local hsdocsMetatable
  hsdocsMetatable = {
    __index = function(self, key)
      local label = (self.__node == "") and key or (self.__node .. "." .. key)
      return setmetatable({ __action = self.__action, __node = label }, hsdocsMetatable)
    end,

    __call = function(self, ...)
      if type(self.__action) == "function" then
          return self.__action(self.__node, ...)
      else
          return self.__node
      end
    end,

    __tostring = function(self) self.__action(self.__node) ; return self.__node end
  }

  hs.hsdocs = setmetatable({
    __node = "",
    __action = function(what)
        local hsdocs = require("hs.doc.hsdocs")
        hsdocs.help((what ~= "") and what or nil)
    end
  }, hsdocsMetatable)
chdiza commented 5 years ago

OK thanks, I'll try that out.

chdiza commented 5 years ago

Holy crap, that is awesome!

asmagill commented 5 years ago

Ok, what I've got so far can be found at https://github.com/asmagill/hammerspoon_asm/tree/master/doc

Despite being in the hammerspoon_asm, it will install as hs.doc in your hammerspoon config dir, which will be loaded instead of the regular one in core.

This still performs the loading of the json files at startup, but it keeps the data entirely in C. I haven't yet added code to defer loading until absolutely necessary, but it's still noticeably faster than before.

The new version also doesn't include the unregisterJSONFile function yet... but honestly, I'm not sure its ever been used, so I'm not sure how important that is.

It generates more warnings then the core version (not sure why yet, I'm pretty sure that parts almost line-for-line the same, just in C), but the standard function, shortcuts, and web server all work. The web server is slow at first, and any time a new documentation json file is added because it's still expecting the json in the lua space, but it only loads it if you start it -- avoid the web server and you never experience the pause. That's going to take more of a re-write than I have time for right now.

@cmsj, I did get reminded of some things (that are also in the core version; I just kept them in for parity) while working on this that we need to decide on:

I'm leaning towards dropping the second point above; it seems unnecessary and if someone is editing their spoon file, they probably also know how to make an updated docs.json file if they need to.

However, I'm undecided about the first... do we still want people to be able to browse spoon docs even before they're loaded? If so, we need to rethink the hs.loadSpoon because most of the time, unless a new spoon has been added or a new spoon directory has been added in the users init.lua, it doesn't need to load the docs. Maybe add a function to hs.doc that determines if the documentation is already loaded?

latenitefilms commented 5 years ago

HS.DOC, The Rewrite: Bigger, bolder, and coming to a thater near you... IN 3D!!!!!

Haha... awesome. However 3D is soooooo 2015.

I haven't yet added code to defer loading until absolutely necessary, but it's still noticeably faster than before.

Great news! FWIW, now that I know how fast it loads without hs.doc loading on reload, I'm pretty keen to only load hs.doc if needed.

Will leave @cmsj to ponder your questions, as I haven't really used spoons properly (yet).

cmsj commented 5 years ago

@asmagill happy to lose the spoon doc rebuilding. On the spoon doc loading - I guess it's possible that someone has HS already running and then drops a spoon into the folder and loads it, which would be good to pull the docs in.

However. I think saving multiple seconds off everyone's startup time is a much larger win proportionately, than the loss of one or two aspects of Spoon doc loading, so I don't mind if we skip a few less important features to get there.

asmagill commented 5 years ago

@cmsj, @latenitefilms, and all others playing the home game...

The potential hs.doc replacement has had some updates:

But the web interface is out of whack -- it no longer sorts items properly and modules which do not have a fully documented element in their path (which I think should only be third party modules at this point; for example hs._asm.undocumented.spaces doesn't show up because hs._asm has no formal documentation entry; the console help sees it but the web site doesn't.

To be honest, I was just happy the web page loaded after my changes yesterday, so I can't say for certain if todays changes caused the latest issues or if I just missed them yesterday; in any case, they're not my priority at the moment, though I hope to have them fixed as well later this week -- I'm trying to avoid a significant rewrite of the web server because it was a pain the first time, but... I think I'm going to have to.

asmagill commented 5 years ago

I forget to mention hs.doc.unregisterJSONFile has also not been implemented yet... in theory, it allows you to unload a specific docs.json file so that it could be re-added if you've made changes and want to see them without actually having to restart Hammerspoon.

I don't know that anyone has every really used it; I never did after implementing it, and I'm the one who initially made the use case for it...

Thoughts on whether it should be implemented or removed?

Never mind - I figured an easy, albeit potentially slow, way to implement this.