search

martinklepsch / boot-garden

Boot task to compile Garden stylesheets.
Eclipse Public License 1.0
37 stars 6 forks source link

support for generating multiple css files: per-var initial state, rem… #8

Closed mecha1 closed 9 years ago

mecha1 commented 9 years ago

…oved :init fn from garden pool

martinklepsch commented 9 years ago

@mecha1 thanks for digging into this.

The :init option allowed us to preload garden in the pod pool shaving of a few seconds for each build cycle. Ideally I would like to keep this behaviour. Have you been able to find the root cause for the exception you were getting in #7?

mecha1 commented 9 years ago

@martinklepsch I'm not clear on how you are expecting :init to shave a few seconds off the build cycle, but from your comment it sounds like you want the init function to be called only once for any number of invocations of the garden task. This is not what is happening. Each time the boot garden task is called, it calls (garden-pool). This means that if you call the boot garden task twice and the :init option is retained, it will call that :init function twice. The exception occurs on the second call to the :init function.

martinklepsch commented 9 years ago

@mecha1 Every time the garden task compiles your stylesheets it uses a new pod from the pod pool. This pod needs to require garden.core itself and your stylesheet var's namespace. The latter is only done after ns-tracker told us that the namespace has changed. In comparison to your stylesheet namespace the garden.core namespace is static and thus we can load it as soon as we create a pod in the pool. That's what :init is for. It allows you to specify something that should be run as soon as the pod is ready and before you actually take it from the pod pool using refresh.

For details see the pod-pool docstring.

Have you not observed any differences in cycle times?

mecha1 commented 9 years ago

@martinklepsch I think there is a discrepancy with the way we are expecting :init to work and the way it is actually working. If you place a print statement in the :init function you will see that the :init function is called when each pod in the pool is first created, AND on every subsequent time you call refresh to take a pod from the pool.

I was originally expecting that the :init function would be called once on pod creation and not afterwards on subsequent pod access, but this is not what is happening. The end result is that instead of saving time by not having to invoke the :init function again whenever accessing a pod from the pool, we are actually wasting more time by using :init than if we didn't use it at all. This feels like a bug in boot (I'm using Boot 2.2.0 by the way). Let me know what you think.

The other issue is that if you are doing two invocations of the boot-garden task, e.g.:

boot watch garden -s mecha1.www.styles/s1 -o s1.css -- garden -s mecha1.www.styles/s2 -o s2.css

it will create the pods and call the :init function on them concurrently, which then causes the exception in #7. I'm still not sure why this is, but it is. If you add a (Thread/sleep 5000) into your :init function it will circumvent this issue, but this is obviously not ideal. It seems preferable to remove the :init option entirely, especially given the aforementioned behavior of the :init function being called too many times.

mecha1 commented 9 years ago

@martinklepsch hmm, ok maybe it was only my misunderstanding. I was expecting boot to reuse pods in the pool, but that is not what it does. It initializes pods (using :init) and then discards them after they are used. So it looks like boot is behaving as expected.

The issue with boot-garden then looks like that it is creating a new pod-pool for each invocation of the garden task. I changed this to a single global garden pod pool and can now avoid the exception in #7 while still keeping the :init functionality. I have updated the pull request accordingly with the new changes.

martinklepsch commented 9 years ago

This makes perfect sense. Thank you.

Would you still consider the exception to be a problem with Boot? If you feel like it a minimal repro would be awesome to file a bug against Boot so we can fix it there :) Seems like the base problem is creating two pod pools with two invocations of the same task.

mecha1 commented 9 years ago

Ok, I'll put something together.

Another thing I noticed is that the using :take instead of :refresh to acquire pod instances from the pool is substantially faster, because :refresh first synchronously calls destroy to clean up the existing pod before returning a new pod instance. It might be a nice feature to have boot return the new pod instance first and then asynchronously destroy the old instance as a performance enhancement.