MithrilJS / mithril.js

A JavaScript Framework for Building Brilliant Applications
https://mithril.js.org
MIT License
14.02k stars 926 forks source link

Make a first-class way for event handlers to be run without auto-redrawing. #378

Closed l-cornelius-dol closed 9 years ago

l-cornelius-dol commented 9 years ago

Allow a noredraw object for event handlers which aren't wrapped in redrawing, or iterate config applying any onxxx attributes in the same way.

Event handling is sufficiently tightly bound to a component that avoiding use of config is preferable, since, ideally, config should be reserved for unusual use-cases, not common ones.

Modify setAttributes to add the following code to the if/else setting attributes:

//handle `noredraw: {...}`
else if (attrName === "noredraw" && dataAttr != null && type.call(dataAttr) == OBJECT) {
    for (var rule in dataAttr) {
        node[attrName] = dataAttr[rule];
    }
}
barneycarroll commented 9 years ago

The more I think about the underlying problem, the more I believe that the general proposal — namely a way of registering event handlers that don't trigger redraw — is really anathema to idiomatic Mithril.

One practical way of achieving something close to the current API proposal is to create an extra, author-written attributes pre-processor to parse a decorated attributes object and return something that can be parsed by Mithril. I wrote a function for this here (the original use case was CSS pre-procesding – reading the style property and extending it with vendor-prefixed sub-properties where necessary). So at a pinch we could combine this with the functional code in #371 to implement the desired functionality (pass in an object with a noredraw map, return an object with a config function that attaches the events and executes any passed in config).

Another approach is to try and grapple with the problem in a more Mithril-sympathetic way and try and develop a mini-module that keeps the interstitial input value in a private VM so that redraw stops being a problem. I can't help thinking that at some point, some external event from an unrelated source could trigger a redraw which would make any redraw-avoiding mechanism like what's being suggested useless anyway.

And this leads me to a third thought: because the proposed aim is to avoid redraws entirely until you're happy the user has finished inputting, could we create a simple module that invokes m.startComputation oninput and m.endComputation on context.onunload or when you've determined the user has finished?

lhorie commented 9 years ago

As I mentioned in #371, you shouldn't rely on redraw prevention as a mechanism to keep the UI in a de-synchronized state, precisely because redraws can be triggered by other parts of the system.

The only valid use case I can think of for a simpler never-redraw utility is when the UI's default behavior syncs with the data model. For example, {oninput: m.withAttr("value", someProp)} does not necessarily need to trigger a redraw if you don't care whether other parts of the screen are up-to-date (note that I said if you don't care, not if you don't want)

This can be achieved generically in app space w/ something like

function noredraw(callback) {
  return function(e) {
    callback(e)
    m.redraw.strategy("none")
  }
}

m("input", {oninput: noredraw(m.withAttr("value", someProp))})

But it can also be made more specific and terse like this:

function setsVal(callback) {
  return function(e) {
    m.withAttr("value", callback)(e)
    m.redraw.strategy("none")
  }
}

m("input", {oninput: setsVal(someProp)})

The core idea though is that the result of redrawing vs not redrawing should not be different in terms of the resulting UI state and data model state. The only difference should be the amount of CPU being used to do "nothing"

l-cornelius-dol commented 9 years ago

@lhorie,

I don't think that m.redraw.strategy() can be used in this way, because while that event handler may not affect the model (either data or view), another handler for the same event might. Indeed that event handler might or might not, depending on the event. So you can't just unconditionally suppress redrawing.

I ran into this very problem with a key handler attached to a list component and another attached to the page body. The one on the list is unable to prevent redrawing when it has done nothing to affect the model; neither can the one at the page level. Both event handlers can say, "this event definitely affected the model and a redraw is required", but neither can say "because this event handler did not affect the model a redraw is not required".

Does that explain the difference?

Because of this it's hard to write self-contained components, because redrawing concerns leak out beyond the component. Whereas, if the event handler did not redraw by default, both could flag that redraw is required and interop perfectly.

PS: You might be noticing that this concern is all about writing components; it's easy to get redrawing working monolithically, but we can't write monolithic apps. We have to be able to create reusable components, and they have to interop with others and the main app.

l-cornelius-dol commented 9 years ago

Another thought about m.redraw.strategy().

It turns out to be very hard to make the claim, at any given moment in time, "there is nothing anywhere in the UI to be updated". Therefore, it's very hard, by extension, to set the strategy to "none".

Conversely, it's easy to make the statement, "the model just changed (I know because I just changed it)", therefore it's very easy to know when to do the moral equivalent of m.startComputation(); m.endComputation(); to trigger a redraw at the next paint cycle.

(Unless I am entirely misunderstanding the effect of setting the strategy, which I could well be; I just know that when I tried manipulating it I ended up with numerous problems of stale UI.)

lhorie commented 9 years ago

Each handler can say "I did not affect the model, so I will not autoredraw". The caveat is that every event handler triggers a separate redraw, so if you have a bubbling event that triggers two handlers and you want neither to trigger a redraw, then you have to call strategy("none") on both handlers.

There's no global way of saying "I don't want to redraw", because every handler may or may not modify the model and there's no way to know if it did unless you redraw.

And again, you should not call redraw.strategy("none") from a handler if it does modify the model, otherwise, yes, you'll run into stale UI problems.

l-cornelius-dol commented 9 years ago

@lhorie,

Each handler can say "I did not affect the model, so I will not autoredraw" [by setting] strategy("none") [within each handler]

My experience and my reading of the Mithril code suggests it does not work this way - from what I see there is no changing of the strategy for each handler, thus if one handler changes it to "none" it's "none" for that event loop period. Thus no event handler can legitimately change the strategy to "none".

Please note that nothing in this asks for a "global" way to stop auto redraw, but a handler specific way to (a) prevent redraw from being assumed (this is needed), and (b) a way signal that redraw should be done because it did, in fact, change the model (this is already there in start/endComputation).

lhorie commented 9 years ago

Here's a jsfiddle: http://jsfiddle.net/oswcande/

On load, you should see one random number in the console (because of the initial render).

If you click "asd", you should see one more number (because there are two handlers, but only only redraws)

If you comment out m.redraw.strategy("none") and click "asd", you should see two new random numbers get printed to console (because there are two handlers and both redraw)

If you call m.redraw.strategy("none") from both onclicks, you should not see any new number get printed to console (because there are two handlers but neither redraw).

If this is not what you've experienced, I'd be curious to see what you're doing. It might be possible that there might be an obscure corner case where it breaks.

l-cornelius-dol commented 9 years ago

I stand corrected!

It's not the way it appeared to be working for me. On Monday, I will restore my key handling to the previous setup, with two handlers like this and see what the result is. I think there was some subtlety I may have messed up.

But my instinct still tells me that the redraw strategy is the wrong tool for achieving this end. It's too broad, and it's use gets really complicated as soon as you have multiple overlapping start/endComputation calls -- that is, it works in this simple case because each event handler happens to result in the handler's endComputation doing a redraw.

What if something else, like a non-background AJAX request started before and ended after the event handling? Wouldn't it then be strategy("none") and fail to redraw.

At the heart of it is that the event handler running is not sufficient reason to redraw; rather the event handler must run _and_ effect a change in the model, which the Mithril code can't know about unless that code tells it.

l-cornelius-dol commented 9 years ago

Consider this fiddle:

http://jsfiddle.net/LawrenceDol/L6hozqbo/

It should redraw, but it doesn't because the upper level handler sets strategy("none"), blocking the subsequent "async" render.

In fact, the more I think about it, the more I am convinced that the redraw strategy cannot possibly be used for managing redraws, since it's a global flag. There is just no way any piece of code can say "the next redraw, whenever that might be, should do nothing" -- no one piece of logic can make that call. It's possible to mark the currently rendered view as dirty, but not possible to change it back to clean; only a render of the current model state can do that.

lhorie commented 9 years ago

Aha. That is definitely not doing what it should. I'll flag it as a bug

l-cornelius-dol commented 9 years ago

@lhorie,

I don't think this is a bug, but rather the entire premise that:

Each handler can say "I did not affect the model, so I will not autoredraw" [by setting] strategy("none") [within each handler]

is flawed. That might be what you intended, but setting strategy("none") says "don't do anything on the next redraw". And what piece of code can possibly make the claim that the next redraw from now should do nothing?

That is much more than what this feature-request desires to achieve. I want to say "don't redraw simply as a result of running this handler" (because it might do nothing); just allow the handler to explicitly indicate if a redraw is necessary, via start/endComputation() or redraw().

lhorie commented 9 years ago

ok, so I think we're more or less on the same page. m.redraw.strategy("none") is supposed to do what you're asking this feature to do (i.e. make a handler not start/endComputation as a default).

The reasons why I didn't make this a special case syntax on the template were a) the strategy abstraction also covers the case of whether route changes redraw from scratch b) I wanted to keep the amount of framework-specific syntax in templates to a minimum

I do concede that the strategy abstraction is confusing though.

l-cornelius-dol commented 9 years ago

So here's an out-of-the-box idea.

Maybe we could leverage JavaScript's everything's-an-object and query the event handler function for a flag that indicates not to wrap the handler, which Mithril queries?

function noredraw(callback) {
    var cbk=function(e) {
        callback(e);
        }
    cbk.mithrilNoRedraw=true;
    return cbk;
    }

Then in Mithril:

function setAttributes(node, tag, dataAttrs, cachedAttrs, namespace) {
        ...
                //hook event handlers to the auto-redrawing system
                else if (typeof dataAttr == FUNCTION && attrName.indexOf("on") == 0) {
                    node[attrName] = dataAttr.mithrilNoRedraw ? dataAttr : autoredraw(dataAttr, node);
                }

Or is that too "magical" for people's taste? Or setting a bad precedent?

l-cornelius-dol commented 9 years ago

Another practical example of idiomatic Mithril spamming redraws is with the stock-standard oninput: m.withAttr("value",...) construct. Since the browser already, and by definition, reflects the change to the model in the view, because the view change is what triggered the model update via oninput, there is no point at all in redrawing as a result of oninput.

In fact doing so probably leads to all manner of problems with selection, cursor position, etc causing edge cases in different browsers. But mostly, regenerating the entire view to render a key typed, which the browser has already done is a complete waste of compute resources.

barneycarroll commented 9 years ago

@lawrence-dol realise the code I linked earlier was broken — I've taken out the relevant bits, fixed the missing bits and written out the noredraw processor, along with a bit of example usage code out of context. Would this help?

barneycarroll commented 9 years ago

For the sake of completeness, I'd like to re-iterate my belief that spamming redraws is a fallacious performance concern. The app I've built is closed source (although I hope to open certain components when I can afford the time to make them suitably generic), so I can't prove this, but I'm listening for onkeyup and onscroll and redrawing the entire app, typically involving creation and destruction of hundreds of elements, and performance is simply not a problem. I'm at a continuous 50FPS.

Let's be clear that redraws are throttled: even if a thousand redraw triggers occur within ~16 milliseconds, you will still only incur one redraw — so even if you have a potentially unlimited recursively nested hierarchy of components with 'naive' Mithril event handlers, they will all bubble up into one redraw.

The long and short of it is that if you are actually suffering performance issues, your problem is elsewhere.

l-cornelius-dol commented 9 years ago

@barneycarroll : That's a lot of code for something that Mithril can provide, integrated, in three lines.

I note that nearly all of this is boilerplate for dealing with the fact that the singular config function doesn't translate well to have separate concerns that want to use the config.

l-cornelius-dol commented 9 years ago

@barneycarroll:

I'm at a continuous 50FPS.

On what hardware? And at what cost of CPU and therefore battery usage? Does your approach scale down to handheld computers?

barneycarroll commented 9 years ago

@lawrence-dol the code is an opportunity to test your assertion that redraws are a performance bottleneck. I'll go that far but I certainly won't go as far as to set up purposefully contrived stress tests with an aim to validating your theory on battery drain — none of which is to say that I wouldn't be interested in seeing any kind of evidence that this might actually be a measurable problem.

l-cornelius-dol commented 9 years ago

Here is a trivial replication of a large, but realistic DOM where auto-redrawing makes key input lag:

http://jsfiddle.net/LawrenceDol/wf6frkr6/

barneycarroll commented 9 years ago

I loaded the demo on my iPhone and couldn't see a problem – but arguably this might be because I can't type fast enough on a mobile device for it to be a problem.

I've modified the test to pull a fork of Mithril:master that logs invocations of m.redraw and the underlying redraw mechanism itself. I then forked the test to log handled events and compare the different scenarios we've been talking about:

  1. Default – functionally identical to your original test, but with everything logged
  2. Redundant handlers – the input and each of its ancestors have the same event handler bound
  3. noredraw – using the fixed implementation I was talking about

I wasn't able to observe any performance difference at this scale, so the problem case doesn't appear to be quite that trivial: by mashing the keyboard I was getting continuous frame rates of ~20FPS and GPU usage of 10.5MB (I'm hovering at ~8FPS as I type this – I assume I just can't hit keys faster than 20 times per second…). Contrast this with something where event handlers trigger more often – [say mousemove] – and everything operates at ~40FPS on my end (again, regardless of variations between the 3 scenarios above).

Things I could observe:

I note that nearly all of this is boilerplate for dealing with the fact that the singular config function doesn't translate well to have separate concerns that want to use the config.

For the record, "all of this boilerplate" refers to all of 58 lines of unminified, liberally-whitespaced code (I even treated myself to using vowels in my variable names) when you exclude convenience functions that should already be available in your functional program utility belt. That's 310 bytes gzipped when you offset the iterators to lodash and put it through closure compiler. The functional code preserves passed-in configs and ensures they are executed as they would normally (I've found separate config functions to be essential in complex UIs) – I tried to highlight this with the use-case code snippet. Here's a demo showing a complex attribute configuration working with the attrs plugin.

l-cornelius-dol commented 9 years ago

@barneycarroll,

This is getting a little off track, so I want to reiterate to make the main points crystal clear:

  1. The cleanest, most concise, and clearest way to add event handlers is using an onxxx attribute.
  2. As I understand it an idiomatic Mithril application entirely drives view off the model (both data-model and view-model). Any any given point in time the view should be able to be rebuilt entirely from the model; the view, that is the vDOM and DOM, holds _no state whatsoever_.
  3. Mithril assumes that every event hooked via onxxx attributes will change the model (in a way that is not already reflected in the view by the GUI), and thus unconditionally implicitly triggers a redraw as a result.
  4. I have found practically, and demonstrated via actual code, that this assumption commonly does not hold for at least several often used events (input, key and mouse), and in principle sometimes does not hold for all events.
  5. I have found practically, and demonstrated via actual code, that redrawing for every key typed causes visible lag in keyed input; just press and hold a key in the fiddle I gave, or cursor back and forth after typing a bunch of characters.
  6. This request is asking for, and only for, a clean first-class way to register event handlers such that the assumption of redrawing is not made, allowing the event handler to explicitly trigger redrawing if, and only if a change to the model is made.
  7. I have offered suggestions for how to do (6), but in no way wish to insist it should be done in any particular way as long as the solution is clean, concise and intentional (i.e. does not have the appearance of being a work-around).

My testing is conducted on a mid-range developer desktop PC, AMD Phenom(tm) II Quad Core 965 Black Edition @ 3400 Mhz with 12 GiB RAM @ 1600 MHz (and a very run-of-the-mill AMD Radeon HD 6450 video card).

_Any_ performance problem seen on my system will be multiplied on my users' systems. My hardware is relatively modest, but it's still 1 to 2 orders of magnitude more capable than my average user's.

With the fiddle I posted, holding a key down jumps from having no observable effect without a key event handler to pegging my CPU use at 26% with the key handler in place. As well, every 2 or 3 keystrokes input pauses and so input is jumpy (I assume this is due to GC). At the same time page faults jump from about 60/sec peaking at 150/sec during input without a key handler to averaging about 1500/sec (peaking as high as 5000/sec) with the key handler.

This lag is occasionally observable with normal touch-typing and downright embarrassing with key-repeat (such as holding down delete or cursoring back and forth).

I have tested this assertion by using config to bypass Mithril's event handler wrapper. With a Mithril onxxx handler keyboad input lags; with a naked handler triggering redraw only if the key is actually handled it doesn't.

Let me be clear on this, I first encountered this shortcoming because field input was visibly lagging. Later it came to my attention again because the cursor position was being reset in IE every time a key was typed.


My objection to adding application code to work-around any shortcoming in Mithril is not chiefly the increased bytes deployed, but increased cognitive load.

I would fully agree with zealously guarding Mithril against bloat; but it's still young and I do think we need to push a little bit to tease out any weaknesses it might have.

I think the incorrect assumption that events always impact the model, and therefore the view, is just one such weakness.

chexxor commented 9 years ago

I may be a little out of order to post, but a third-party perspective seems appropriate to balance the heated discussion.

@lawrence-do - I appreciate your effort to create and defend this issue, as this topic seems pretty important. Your point is very clear, and you've created a convincing argument that Mithril's redrawing needs more configurability. Very awesome stuff. :+1:

@barneycarroll's point seems to be "redrawing is cheap, so just don't worry about how often it happens". For Mithril newcomers, that is a fine stance. However, every sufficiently-advanced app needs a way to customize the complex part of a library which manages complex stuff.

I find this behavior very important knowledge.

barneycarroll commented 9 years ago

I think the incorrect assumption that events always impact the model, and therefore the view, is just one such weakness.

This might sound like a pedantic correction, but I think it's significant: Mithril assumes events bound in the view may have changed the model. Furthermore, idiomatic Mithril offers a smaller API surface than previous MVC libs by removing the entire cognitive load of how and when to redraw by default. The underlying assumption — which is a design decision — is that Mithril's redraw process is performant enough such that only exceptional circumstances would cause a developer to want to interfere with it.

Your argument is that the redraw process is not performant enough, to the point where it is a conceivably common enough despite for developers to opt out of automatic redraws, and that the extra cognitive load of redraw-conscious event binding should be eased by a more accommodating API.

What seems disingenuous in all this is that you're not interested in even commenting on the observable effects of my implementation of the API you suggested — which, up til now, has been the only attempt at validating your hypothesis about performance.

You suggest that bad performance in a given fiddle using current Mithril code and your own application code is proof of bad performance in Mithril. I'm saying that's fallacious reasoning, because the application code could be the sole significant source of performance overhead. The only way of proving the hypothesis is by comparing this to an implementation that sidesteps the allegedly problematic code in Mithril, and then highlighting the resulting performance improvement. This is why I produced the extra fiddles — and why it's significant to note that avoiding redraws does not measurably affect performance.

Now you might reasonably want to question my evidence and argue that the plugin code I wrote is internally suboptimal and adds overhead in its own right by introducing extra function calls when your end goal is to reduce just that, or that I'm not measuring evidence properly, and you'd like to see a comparison based on other metrics (CPU usage — let's go with that). If that's the case, we could fork Mithril to internalise the essential logic of my plugin and see whether that produced tangible results. Would that be a relevant contribution, or am I still missing the point?

l-cornelius-dol commented 9 years ago

@barneycarroll,

It is a pedantic correction; in assuming the event may have changed the model, Mithril proceeds to act as if it did change the model. Therefore the assumption as coded is that every handler does affect the model.

I have commented repeatedly on the observed effects, having originally observed them in my application (and reproduced them in a trivially simple fiddle), and having worked around them by using config + addEventListener.

Compare this fiddle, using the Mithril handler:

http://jsfiddle.net/LawrenceDol/wf6frkr6/

to this one, identical in every way, but using a bare HTML handler:

http://jsfiddle.net/LawrenceDol/cpsx31f1/

by pressing and holding a key down in the input field. Input is visibly lagging, CPU jumps to 100% (of one core), and memory load goes off the charts. Attaching an input event handler to the input field does the same thing. All because the view is being regenerated for no reason other than a mistakenly broad assumption.

How is that _not_ something which should be targeted for improvement? Or do you truly mean to say I should nowhere in my application, ever use a key, mouse or input handler with a moderately complex view?

In these fiddles there is no application code, therefore your suggestion that it might be my application code causing the problem is moot. These are not assumptions, these are observed effects, and this issue was raised as a result of having encountered them and worked around them.

l-cornelius-dol commented 9 years ago

@barneycarroll,

Whatever version of Mithril you have used for your fiddles is sufficiently different that the key-lag effect is not very obvious, though the stress on CPU and memory is still very clear.

I am, on principle, using the 0.1.26 version from Mithril's GitHub.

lhorie commented 9 years ago

FYI, I just pushed a commit that changes the semantics of strategy("none") to cover the edge case that @lawrence-dol pointed out. Calling it now disables synchronous redrawing, but has no effect on asynchronous redrawing, and therefore it can no longer "contaminate" unrelated event handlers. This fixes the issue in @lawrence-dol's jsfiddle

I've also updated the documentation with the hopes that it makes it intended behavior clearer.

l-cornelius-dol commented 9 years ago

@lhorie,

Much less of problem, but the current behavior now appears to redraw once for every handler in the DOM tree.

Fiddle: http://jsfiddle.net/LawrenceDol/xogvkc70/

With both m.redraw.strategy calls commented out the draw counter clicks up by 2 each click. With either commented out, by one. And with neither commented out, by 0.

Maybe that's the intent, but most desirable would be a single repaint on the next event loop cycle.

(BTW, I have extensive background in GUI and graphics systems, so I am keenly aware, more than most, of the high cost of excessive layout and painting in rendering systems; and too, of the need to optimize by marking "dirty" regions rather than trying to ascertain that all regions are "clean".)

tivac commented 9 years ago

@lawrence-dol https://rawgit.com/lhorie/mithril.js/next/mithril.js (rawgit.com serves files straight from github w/ proper content-type headers, it's great for testing things like this)

barneycarroll commented 9 years ago

Ignore the fork referenced above, it was badly re-based. @lawrence-dol I can try patching this onto 0.1.26 if you want. BTW redraw doesn't touch the DOM unless the output of a view has changed orm.redraw.strategy has been set to none. There is not supposed to be any repaint / layout re-calculation unless the model data nodes invoked by the view have changed — if there is, that is a far more pertinent bug. You can test for this by extending your fiddle of choice with a mutation observer on the html node to log any modifications to the subtree. This is why I've been keen to stress that the key comparison to make is between the process-blocking potential of a no-op diff calculation vs that of the event handlers themselves.

lhorie commented 9 years ago

@lawrence-dol deferring the redraw to the next event loop was causing some other issues (namely, it opened up room for a race condition between auto-redrawing and user input that could make users intermittently lose keystrokes in cases where both fast typing and expensive redraws were involved.)

I don't know if you saw the article, but I wrote about AOP-style template extensions here. The technique there (and variations of it) can be used to make certain event types always be no-redraw by default.

For example, one could start building a DSL for forms and add the optimizations into that layer, e.g.

function input(attrs) {
  var input = attrs.oninput
  if (typeof input = "function") {
    attrs.oninput = function(e) {
      m.redraw.strategy("none")
      input(e)
    }
  }
  return m("input", attrs)
}

//usage
form([
  label("name"),
  input({oninput: doStuff})
])

Another avenue that we could start looking into again is the idea of local modules (basically #291).

barneycarroll commented 9 years ago

@lawrence-dol FYI I fixed my noredraw branch (re-based off next). It implements the original API you suggested, so you might get some interesting readings from it. For the sake of fiddling, these are the rawgit URLs of the last version of lhorie/mithril.js#next and barneycarroll/mithril.js#noredraw at the time of writing.

@lhorie for clarity, does anything actually happen at all, in practical terms, as a side-effect of tripping autoredraw but then immediately setting strategy to none – or is it a total no-op?

lhorie commented 9 years ago

@barneycarroll Not sure I understand your question. Do you mean, does the following consume any CPU on click?

m("div", {onclick: function() {m.redraw.strategy("none")}})

No, in that case, the only thing mithril does is reset the strategy flag and decrement the computation counter in that case. No redraw happens.

barneycarroll commented 9 years ago

The solutions I've been working on so far try to avoid invoking redraw at all, whereas your proposals hinge on cancelling autoredraw by invoking m.redraw.strategy. I'm just trying to confirm that this is a matter of convenience (less convoluted code) rather than a practical consideration.

l-cornelius-dol commented 9 years ago

@lhorie,

I tested my fiddle with the "next" Mithril, and my earlier comment holds; using the redraw strategy it now does one redraw for every handler that executes and doesn't set "none".

Just food for further thought; I consider this issue resolved.

l-cornelius-dol commented 9 years ago

@barneycarroll,

While the view creation and diff is much lighter weight than recreating (parts of) the DOM, it is most certainly not a "noop", and in my experience (en toto, not just Mithril & JS) it would be naive to consider it so.

Any complex application will do significant work in view generation, especially if the idiomatic Mithril use of templates is followed (and I think it should be).

My fiddles demonstrate that; the DOM for the large table should not be being touched, though I agree we should confirm that is the case.

(We also need to constantly bear in mind that we developers are typically running "thumping great big" machines compared to our users.)

lhorie commented 9 years ago

I'm just trying to confirm that this is a matter of convenience

The considerations I'm taking into account are:

it now does one redraw for every handler that executes and doesn't set "none".

Yes, this is expected behavior. Not necessarily ideal, but expected. To support the ideal situation, I need to be able to wrap code around the browser's internal event loop, which is not possible, so this (plus the ability to set strategy to none on a per handler basis) is the next best compromise given the problems that arise from alternative solutions.

l-cornelius-dol commented 9 years ago

@barneycarroll,

I altered the fiddle to log if the table DOM is recreated and it logs only once at first-draw. Unless I've made a mistake, it now proves that the lag and system stress is caused purely by view generation.

l-cornelius-dol commented 9 years ago

@lhorie,

It would be good to keep this percolating... What concerns me is that with any large view (in terms of DOM nodes), adding a key, mouse or input handler anywhere will cause excessive stress and visible key lag.

But I routinely add input handlers to input fields to generate change events when the user pauses typing rather than wait until they press ENTER or exit the field. And I nearly always have module-level key handlers to detect and action control keys (ENTER, ESC, etc).

I've enhanced this fiddle to show view creations and prove that the table DOM is not being reconstructed, and it clearly shows that every key typed into the input field causes three view generations. That's a really surprising side-effect for a Mithril new-comer. Worse, it's likely to go unnoticed until the application deploys and deals with real data-sets (like the perennial problem of developers testing with tiny databases and deploying to systems with hundred-thousand record databases).

Moreover, while the fiddle creates a large number of DOM nodes, they are trivially simple ones, much more so that a real application, which will create links, and buttons, inputs, etc, all of which are more complex in terms of view objects.

For this reason, I am still in favor of Mithril having a clearly visible, first-class way to register event handlers which are not redraw-by-default. Please continue to consider this more deeply (and maybe reconsider re-opening this issue?).

barneycarroll commented 9 years ago

Another fun situation, maybe with too much of my own code to qualify as a legitimate problem:

function multi(){
    var handlers = [].slice.call( arguments );

    return function execute(){
        var args = [].slice.call( arguments );
        var ctxt = this;

        handlers.forEach( function applyCtxt( handler ){
            handler.apply( ctxt, args );
        } );
    };
}

var onKey = ( onKeyClosure(){
    var keymap = {
        'enter' : 13,
        'space' : 31,
        'tab'   : 9,
        'esc'   : 27,
        'left'  : 37,
        'up'    : 38,
        'right' : 39,
        'down'  : 40
    };

    return function bind( key, callback ){
        if( key in keymap ){
            key = keymap[ key ];
        }

        return function handler( e ){
            if( e && key === e.keyCode || key === String.fromCharCode( e.keyCode ) ){
                callback.call( this, e );
            }
            else {
                m.redraw.strategy( 'none' );
            }
        };
    };return 
}() );

m( 'form', {
    onkeydown : multi(
        onKey( 'esc',   discard ),
        onKey( 'enter', save )
    )
} );

The composed handler will never redraw. More food for thought — I don't have any intention of using any redraw-avoiding code, but it's interesting to see how this scales.

barneycarroll commented 9 years ago

@lawrence-dol BTW, what were your reasons for dropping your original noredraw API extension suggestion?

l-cornelius-dol commented 9 years ago

@barneycarroll,

BTW, what were your reasons for dropping your original noredraw API extension suggestion?

I have conceded that Mithril as it's now operating (as in for the build in "next") has a work-around to the underlying problem; but I am not convinced that my original suggestion should not go forward:

For this reason, I am still in favor of Mithril having a clearly visible, first-class way to register event handlers which are not redraw-by-default. Please continue to consider this more deeply (and maybe reconsider re-opening this issue?).

For Mithril up to 0.26, when I had (Mithril registered) key handlers at various levels in the DOM they interfered with each other in such a way as to make the redrawing on every key press impossible to avoid. So I had to install a single page handler and have it delegate to component subhandlers (which prevents proper componentization). But then having an input handler hosed that, so I ended up using config + addEventListener, which is where my code is at now.

I am of two minds about changing it to use m.redraw.strategy("none") because it still feels like the wrong tool for the job -- that is, it feels like it works more by good luck than by good design.

I would much rather have a built in noredraw object, and a new method that would schedule a redraw event if one is not already pending, and set a flag that redraw is required, which flag would be cleared on every redraw and tested by the redraw event. Because I think that this is the only way that interoperable components can be created.

But I am trying to avoid conflating the underlying requirement with a specific solution.

l-cornelius-dol commented 9 years ago

@barneycarroll,

The example you just gave is an excellent example of why m.redraw.strategy("none") is simply the wrong tool for the job.

Because, once again, the core direction of "redraw unless told not to" just doesn't interop properly with separate bodies of code -- it requires tight coupling, though with the new changes that coupling is limited to code running within one event handler. Your example just illustrates how a perfectly good abstraction can end up violating the design with what appears to be a perfectly legitimate use. And this is particularly disappointing, because your abstraction is elegant, clean, very clear and easy to use.

GUI/rendering systems generally cannot work with a draw-unless-suppressed design unless you can make that suppress flag granular down to the individual GUI subtree. If the desire is, like Mithril's design, to have a top-down total refresh, you need to have a nodraw-unless-flagged system in place with the draw actually deferred into the event loop.

barneycarroll commented 9 years ago

@lawrence-dol the problem with my code above is that it makes brittle assumptions about the Mithril event loop. The solution is to make event-handling code generic enough such that it makes no explicit invocations of the Mithril redraw API, while still allowing intent to be inferred by a higher-order handler which takes responsibility for interpreting outcome.

Taking a leaf out of jQuery's book, we could use return false in any given event handler as an inference that the event should be interrupted. This is that bit more elegant because it applies just as well to the native event loop – this code could be applied outside of a Mithril context and work just as well – without decorating the event object itself. The multi function can be re-written to accommodate this loose API and would work equally well with native DOM interaction, jQuery code, or completely unrelated code (eg binding multiple plugins to config).

function event( handler ){
    return function bound( e ){
        var outcome = handler.call( this, e );

        if( this && e && e instanceof window.Event && this[ 'on' + e.type ] === bound && outcome === false ){
            e.preventDefault();
            e.stopPropagation();

            m.redraw.strategy( 'none' );
        }
    };
}

function multi(){
    var handlers = [].slice.call( arguments );

    return function execute(){
        var args = [].slice.call( arguments );
        var ctxt = this;

        return handlers.reduce( function applyCtxt( previous, handler ){
            return handler.apply( ctxt, args ) === false && previous === false;
        }, false );
    };
}

var onKey = ( function onKeyClosure(){
    var keymap = {
        'enter' : 13,
        'space' : 31,
        'tab'   : 9,
        'esc'   : 27,
        'left'  : 37,
        'up'    : 38,
        'right' : 39,
        'down'  : 40
    };

    return function bind( key, callback ){
        if( key in keymap ){
            key = keymap[ key ];
        }

        return function handler( e ){
            return ( e && key === e.keyCode || key === String.fromCharCode( e.keyCode ) ) ? callback.call( this, e ) : false;
        };
    };
}() );

// application code 

m( 'form', {
    onkeydown : event( multi(
        onKey( 'esc',   discard ),
        onKey( 'enter', save )
    ) )
} );

m( 'form', {
    onkeydown : event( onKey( 'esc', discard ) )
} );

Not sure if this makes working with the current redraw loop functionality any more viable.

l-cornelius-dol commented 9 years ago

@barneycarroll,

But you still have to admit, the added complexity is a result of Mithril's underlying assumptions; this entire scenario would be much simpler if only the event handler were not wrapped in start/endComputation. Hence my argument that providing a clean way to do that is desirable.

BTW, your reduce function is wrong, I think; it should be:

return handlers.reduce( function applyCtxt( previous, handler ){
    return handler.apply( ctxt, args ) || previous;
}, false );

The overall outcome is true if any one of the event handlers returns true.

barneycarroll commented 9 years ago

@lawrence-dol you're probably right.

The overall outcome is true if any one of the event handlers returns true.

That was the intention — if any one handler wants a redraw, it takes precedence.

Bear in mind you could kill all the conditional propagation logic with a single flag at the root:

function eventHandler( handler, redraw ){
    var noRedraw = arguments.length > 1 && !redraw;

    return function bound( e ){
        var outcome = handler.apply( this, e );

        if( noRedraw || this && e && e instanceof window.Event && this[ 'on' + e.type ] === bound && outcome === false ){
            e.preventDefault();
            e.stopPropagation();

            m.redraw.strategy( 'none' );
        }
    };
}

Do you reckon multi-instance Mithril (with separate redraw loops) is the way forward then? TBH I think this could cause performance issues of its own and I can't imagine my ever wanting to use it myself.

BTW am I right in thinking you have an app that involves mousemove logic? I'm trying to think of a credible stress test that really makes redraw performance tangible, and I seem to remember you demoing a drag & drop widget scenario…

l-cornelius-dol commented 9 years ago

@barneycarroll,

That was the intention — if any one handler wants a redraw, it takes precedence.

Oh, I misread the effect of a false outcome; you use false to mean handled, so stop event handling (BTW, I never use stopPropagation; it invariably causes a bunch of interaction problems --I use defaultPrevented to indicate that the event has been handled.) But since false means "handled" then it should not set strategy "none", since the assumption is that if the key was handled the model was affected; so I think your code does something wrong there.

Do you reckon multi-instance Mithril (with separate redraw loops) is the way forward then?

I've not been thinking in that direction, and my experience with writing a full GUI toolkit suggests not.

But the most significant thing that I am wrestling with in using Mithril right now is figuring out how to componentize things. Reusable components is key to making the building blocks that increasingly streamline development.

At present it feels like my Mithril code is too tightly coupled to the controller. It might be that I would just prefer a pure component model over MVC. So far the only things I have successfully decoupled are specific views, almost as if a component should be VVM only.

BTW am I right in thinking you have an app that involves mousemove logic?

No, though mouse events can really end up spamming any drawing system. But my fiddle with key and input handlers (which I am using, in essentially the same way as the fiddle) adequately demonstrate real, substantive problems with drawing. Are you not seeing the same problems?

barneycarroll commented 9 years ago

you use false to mean handled, so stop event handling

OK, I got horribly confused there for a second — the top-level event function code I posted is actually downright wrong in its conflation of preventing default behaviour and preventing redraw. If I get rid of the native event API interaction, it makes some kind of sense:

function event( handler, redraw ){
    var noRedraw = arguments.length > 1 && !redraw;

    return function bound( e ){
        var outcome = handler.apply( this, e );

        if( noRedraw || this && e && e instanceof window.Event && this[ 'on' + e.type ] === bound && outcome === false ){
            m.redraw.strategy( 'none' );
        }
    };
}

The idea behind the return false stuff is that you can ensure some kind of upwards messaging protocol for nested callbacks and leave it up to the higher-order wrappers to handle it. And we can use this to express something akin to 'in terms of application logic, nothing actually happened here'.

To put it another way, you could argue that the real problem is that onkeypress isn't granular or expressive enough to be meaningful in terms of your application's internal logic —but Mithril makes the assumption that it is. None of this would be a problem if we had specific event handlers for onescapekeypress because that would be directly relevant. What I'm trying to do here is create a contract in all 'conditional' DOM event handlers that says they should give some feedback to indicate that the meaningful event didn't actually take place. So return false is kind of like 'false positive'. If you've got multiple conditions and all of them resolve to 'false', it's effectively saying that there was no 'real' event in terms of app logic, so act as if nothing happened (but if any single one of them doesn't, then it's business as usual and you should probably redraw as normal).

The top-level handler feeds this back to Mithril, not only because it's only then that we can make conclusive judgments as to the net effect of that event trigger, but also to liberate sub-handlers from implementation details.

barneycarroll commented 9 years ago

Are you not seeing the same problems?

No. It was all a bit "it doesn't seem like things are particularly show", so I cracked out Chrome's FPS / GPU measurements. But then I found higher FPS by triggering redraws on mousemove, so that clearly wasn't a good indicator. The lack of standard measurements and tools for perf quantification is really annoying.

l-cornelius-dol commented 9 years ago

@barneycarroll,

In general FPS is a measure of the work that system is capable of; that FPS goes up with redraws of mouse-moves is expected because you are increasing the number of frames requested, and presumably your system is capable of producing them.

But a UI should request frames at the exact minimum rate required to show the desired visual effects and absolutely no more. Repainting a display, at any level, when it has not changed is wasteful.

Making the system produce 60 FPS when all that is required is 10 FPS to reflect the actual changes is undesirable; you are producing 50 frames per second unnecessarily. Especially on modern hardware where that will pull the CPU, GPU and memory subsystems out of idle clocking and ramp up their power consumption.