Skip to content
This repository was archived by the owner on Jan 11, 2020. It is now read-only.

Implement basic clients method for tag#578

Closed
AdminXVII wants to merge 1 commit into
way-cooler:masterfrom
AdminXVII:tags-clients
Closed

Implement basic clients method for tag#578
AdminXVII wants to merge 1 commit into
way-cooler:masterfrom
AdminXVII:tags-clients

Conversation

@AdminXVII
Copy link
Copy Markdown
Contributor

@AdminXVII AdminXVII commented Oct 20, 2018

Create a clients method as well as a client initialization mecanism in the tag constructor to allow to set the clients.
A downside of this implementation is that clients are stored as values rather than references, so they are not the original Lua values.
Small changes were made in the clients file to allow tests while the root API is not finished.

I am fairly new to rust, so some code may not be optimal, sorry.
As said in the commit message, to keep TagState lifetime-free and keep the Tag object in the standard format, clones are stored instead of references. This in turn means that the clients are not the original lua values. If you see a workaround, please let me know, so I can improve the code (and my knowledge).

Related: #394

Create a clients method as well as a client initialization mecanism in the tag constructor to allow to set the clients.
A downside of this implementation is that clients are stored as values rather than references, so they are not the original Lua values.
Small changes were made in the clients file to allow tests while the root API is not finished.
@AdminXVII
Copy link
Copy Markdown
Contributor Author

This method does not allow client to have multiple tags so it is not a good option.

The other way I see is keeping a global registry of clients and simply keeping a vector of references to clients in tags and vice-versa. We however have to make sure that the two are always updated together. It is a hacky and bug-prone implementation in my point of view.

@AdminXVII AdminXVII closed this Oct 20, 2018
@psychon
Copy link
Copy Markdown
Contributor

psychon commented Oct 20, 2018

simply keeping a vector of references to clients in tags and vice-versa.

In awesome, a tag has a list of clients. To get the list of tags for a client, the C code iterates over all tags and for each of them checks if the current client is on that tag. O(n^2), but seems to work well enough in practice.

@AdminXVII
Copy link
Copy Markdown
Contributor Author

AdminXVII commented Oct 20, 2018

Yes, this is why my implementation was made as such.
However, the tags store references to the client, so they can 1) return exactly the same instances they aquired 2) share the clients
I do not know a way of implementing this without adding lifetimes to the TagState and without changing the Tag object format.
If you have an idea about how to do it, please share, I think it would be the best option.

@Timidger
Copy link
Copy Markdown
Member

Hi, just seeing this now.

Note that in Way Cooler we generally are storing linked Lua objects in the Lua table of the objects themselves (for example, here is the drawin storing the drawable in its Lua Table, instead of storing the DrawableState in its DrawinState).

The reason we do this is due to how rlua works. There are other issues about this, but here's a direct reply to me mentioning why it can't work. tl;dr: GC and lifetimes make things funky.

However this exact approach won't work for your case either, since Tags need to "share" clients. So this is crying out for an Rc. Tbh it's been a few months since I worked on Way Cooler so I'm not sure what the ramifications of simply adding Rc<Client> to the Tag lua table would be (i.e doing exactly like we do in the drawin/drawable case, but with a reference counter).

If that doesn't work, we can use light user data, though that will require unsafe casts and us to manage the memory carefully. This is what C Awesome does, by managing the Lua unref manually. I'd prefer to not have to do that, for obvious reasons.

One option is to have a HashMap<Tag, Rc<Client>> in a global. That would solve the performance problems (O(1) lookup) and the issue of having to associate them with the weird lifetime requirements (though you still may run into issues).

I'll have to look into this in detail later when I can actually work on the code (~2 weeks left, woot woot) but feel free to take a crack at it but don't feel discouraged if you run into annoying issues.

btw @AdminXVII thanks for working on this and the other patches you've sent 😄

@AdminXVII
Copy link
Copy Markdown
Contributor Author

OK, thanks for the reply and for the ideas, I'll try and see what I can do on those paths.
For the patches, it's my pleasure, I can't wait to have the equivalent of AwesomeWM on Wayland, so I'm glad if I can help achieving that.

@Elv13
Copy link
Copy Markdown

Elv13 commented Oct 22, 2018

IMHO the tags can be implemented in Lua for way-cooler and add a thin private API to communicate to the "real" way it is implemented behind the scene. The reason tag is implemented in C by AwesomeWM is purely because it is originally a fork of DWM and that design choice survived all the way to 2018. AwesomeWM-X11 could with some work pry it out of the Core API (CAPI) and only keep the X11 workspace ID, but that's non trivial after all these years. It would also slightly break the API because the sequence of event wont be exactly the same and type() also wont behave the same. This isn't an issue for Way-Cooler since if we target Awesome 5.0 such minor changes in behavior are totally acceptable as long as they are documented.

So maybe replicating the Awesome internals here isn't the best solution. It could be simplified a lot with no downsides.

As a proof of good will, I will now remove what /is/ trivial to remove from CAPI and the lowest hanging fruit is skip_taskbar. It is implemented in C because it predates the generic client property API (edit: arg, no, there is an xproperty, it will require some more work to use the X property API~~) (object.set_newindex_miss_handler / object.set_index_miss_handler, which themselves are a hack to avoid breaking the API. A cleaner solution would be to always expose userdata using setmetatable({}, {__index = setmetatable(lua_object_class_impl, {__index = userdata})}), but that's another topic.

edit2: Actually, the tag is easier to extract than skip_taskbar

-- Deeply simplified and incorrect, but "does things"

local _tag = {_repeat = function(...) --[[implement connect_signal]] end}

_G.tag = setmetatable(_tag, {__call=function(_,...)
    local ret = gears.object { enable_properties=true, enable_auto_signals=true}
    gears.table.crush(ret, {name="atag", activated=false, selected=false})
    ret:_connect_everything(_tag._repeat)
    return ret
end}) 

client.connect_signal("new", function(s)
    s._tags = {}
end)

awesome.register_xproperty("_NET_WM_DESKTOP", "number")

client.connect_signal("new", function(c) 
    local workspace = c:get_xproperty("_NET_WM_DESKTOP")
    
    c._tags = {c.screen._tags[workspace]}

    print("Expect", workspace)
end)

function awful.client.object.tags(c, tags)
    c._tags = tags or c._tags
    return c._tags
end

@AdminXVII
Copy link
Copy Markdown
Contributor Author

AdminXVII commented Oct 22, 2018

The problem with using lua as the main programming language for the implementation is speed. I do not know specifically for Lua, but generally dynamic and weakly typed languages are more than an order of magnitude slower than what would Rust/C be. AwesomeWM already takes like 30s to launch on my computer (yes, my config is lengthy), I cant imagine if the binding where 10x slower.
Unless we use LuaJIT, but it is falling behind regular Lua versions and is not used by RLua

@Elv13
Copy link
Copy Markdown

Elv13 commented Oct 22, 2018

I cant imagine if the binding where 10x slower.

The implantation above would be faster.

LuaJIT is an exception to this rule. Also the number of Lua<->Core interface is the same, so the argument is mostly theoretical. (edit: well, yeah, some will point out there is an extra round trip to the server, but once, not every single time the property is accessed).

But the important detail here is that LGI is absurdly slow and nobody cares. It is many order of magnitude more widely used than the tag C interface. The idiotic way I implemented the custom tag property miss handler is also slower than the implementation above. So from both "it doesn't matter because it's not critical code" and "even if it would indeed be slower, which it wont be, it's would not even reach the top 100 bottleneck".

There is /no/ way porting tag to Lua will have any visible impact on performance.

AwesomeWM already takes like 30s to launch on my computer (yes, my config is lengthy)

Holy WTF Batman! I got the largest config there is an I still manage to make it start below 1sec on a 6 years old laptop. Do you use an Intel 486? Can you upload your config somewhere so I can take a look or at least provide a screenshot and more information. Unless you dismiss all the warnings about never using blocking I/O I sparkled the documentation with, there is no way 30 seconds can be a thing without some major aberrations hiding somewhere. I want to know more ;)

@AdminXVII
Copy link
Copy Markdown
Contributor Author

AdminXVII commented Oct 22, 2018

Nothing too fancy, with a modern laptop (i5-8250U, 8GB of RAM) (I suspect that the HDD is a problem)
Heavily based on the Holo theme found on awesome-copycat
oct22 175312
rc.lua.gz
theme.lua.gz
I am surprised it launches that fast on your computer though, as even the base config takes like 5-10s.

But to the main point, ok, I did not know that way-cooler used LuaJIT, so yeah sorry, a JIT compiler would slow only like the two first calls, plus being not that critical, so point dismissed.
And it is true that it would be cleaner so there would be less chance to make the implementation not optimized because of useless calls burried in the code, so this could also help the balance toward lua.

@Elv13
Copy link
Copy Markdown

Elv13 commented Oct 22, 2018

I did not know that way-cooler used LuaJIT

Lua 5.1 and LuaJIT are mostly drop-in replacement for each other. I don't think it uses LuaJIT by default, but it isn't a factor here. The overhead of the current tag API is larger than a pure Lua one because of the useless calls burried in the code (as you call them).

I suspect that the HDD is a problem

Probably, but my best guess is that it could be made to start under 2 seconds. 4200RPM laptop drives are a large source of latency and loading, say, the menu will take seconds, but Awesome itself should start faster. I don't have time this week and I am out of town next week, but if you are in Montreal we can meet and I can take a look. That would be faster than debugging over the Internet. Getting rid of freedesktop.menu.build which is an horrible thing will already cut the startup time by half.

@AdminXVII
Copy link
Copy Markdown
Contributor Author

Awesome, thanks for the tip!! Almost everything was due to freedesktop! Now it takes under one second to load.

@Elv13
Copy link
Copy Markdown

Elv13 commented Oct 22, 2018

<offtopic args="left's more on now">

Awesome, thanks for the tip!! Almost everything was due to freedesktop! Now it takes under one second to load

Yeah, I once fixed the code of that module and made it (correctly profiled) take 6% of the original time. Then I rewrote it for my config using GLib own internal cached implementation and made it take 0% (!) of the old freedesktop module load time by leveraging async calls. That's a pitty. Both Awesome own implementation and the various other implementations are all better than the freedesktop module shipped by copycat-killer and Debian. It's just the worst possible implementation. It's: 1) blocking, 2) incorrect and 3) absurdly inefficient.

On SSD you would not notice since "low volume" random reads are fast enough as long as they are not sustained. On HDD it's the worst possible access pattern. There is tons of optimizations to avoid your issue. The freedesktop implements none of them and abuse of I/O in order to keep the code "simple".
</offtopic>

@psychon
Copy link
Copy Markdown
Contributor

psychon commented Oct 23, 2018

I did not know that way-cooler used LuaJIT

Lua 5.1 and LuaJIT are mostly drop-in replacement for each other.

Way-cooler uses rlua and rlua is specific to Lua 5.3. This is written in Rust, so it can not use all the macro-magic that Lua does in order to keep its C API look similar over the versions, remember Lua breaks its ABI all the time (and Lua 5.1 and LuaJIT are not ABI-compatible either).

@Elv13
Copy link
Copy Markdown

Elv13 commented Oct 23, 2018

My bad, the first "rlua" result on Google says 5.1, but apparently it's "r" for ruby and not rust. I didn't pay attention.

@AdminXVII AdminXVII deleted the tags-clients branch October 25, 2018 01:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants