Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove "global" library state #5

Open
jamesplease opened this Issue Jan 24, 2016 · 19 comments

Comments

Projects
None yet
4 participants
@jamesplease
Copy link
Contributor

jamesplease commented Jan 24, 2016

This library only supports one instance per page due to the way it is written with global library state. A more idiomatic approach would be to expose a constructor that makes a new MainLoop. For instance,

var mainLoop = new MainLoop(options);

mainLoop.setEnd(cb);

// You can easily make another one
var mainLoopTwo = new MainLoop(options);

Given how well-organized the code is, I think it'd be relatively simple to refactor it to use this system instead.

If this is something you're interested in, let me know and I'd be happy to put together a PR for you to review.

@IceCreamYou

This comment has been minimized.

Copy link
Owner

IceCreamYou commented Jan 24, 2016

Yeah, I did that intentionally because having more than one main loop running per thread makes it easy to make mistakes when multiple loops touch shared state. It's called a "main" loop for a reason. 😃

So, I recommend following a pattern like the one used in the demo, where there is one "main" function that calls other functions when work needs to occur. Here is a simple example:

MainLoop.setUpdate(function(delta) {
    updateThingOne(delta);
    updateThingTwo(delta);
    for (var i = 0, l = otherThings.length; i < l; i++) {
        otherThings[i].update(delta);
    }
});

This is more performance-friendly than having multiple loops, and I think being this explicit makes shared state mistakes less likely.

If you need to attach and detach arbitrary functions, you can set up a registry (otherThings in the example above) and call them from within your dispatch function. Or even something like this:

var begins = [],
    draws = [],
    updates = [],
    ends = [];
function MyLoop() {
}
MyLoop.prototype.registerBegin = function(begin) { begins.push(begin); };
MyLoop.prototype.registerDraw = function(draw) { draws.push(draw); };
MyLoop.prototype.registerUpdate = function(update) { updates.push(update); };
MyLoop.prototype.registerEnd = function(end) { ends.push(end); };
MainLoop.setBegin(function(timestamp, frameDelta) {
    for (var i = 0, l = begins.length; i < l; i++) begins[i](timestamp, frameDelta);
});
MainLoop.setUpdate(function(timestep) {
    for (var i = 0, l = updates.length; i < l; i++) updates[i](timestep);
});
MainLoop.setDraw(function(timestep) {
    for (var i = 0, l = draws.length; i < l; i++) draws[i](timestep);
});
MainLoop.setEnd(function(fps, panic) {
    for (var i = 0, l = ends.length; i < l; i++) ends[i](fps, panic);
});

All that said, I would be interested in hearing more about any use cases where making MainLoop instantiable would make the library more convenient to use. For example, perhaps there are legitimate cases where having different physics timestamps would be useful, or where modules with different privileges make sharing a global update callback impractical. If there are compelling use cases here, I think I could be convinced that making MainLoop instantiable would be a good thing.

@IceCreamYou

This comment has been minimized.

Copy link
Owner

IceCreamYou commented Feb 4, 2016

I would be interested in hearing more about any use cases where making MainLoop instantiable would make the library more convenient to use.

In the absence of feedback on this, I am closing this issue, but feel free to reopen if there are compelling use cases to add.

@IceCreamYou IceCreamYou closed this Feb 4, 2016

@bigsweater

This comment has been minimized.

Copy link

bigsweater commented May 10, 2016

@IceCreamYou

Hi there,

I may have a use case that merits instantiated MainLoops.

I'm building a web app with a JS library (using Vue). One of my components contains a canvas element, which I've animated using MainLoop.

With Vue, I can add more than one of these elements to a page. (You can see an example of the element in use on Codepen.) When I add new ones to the page (called by dropping a new <starfield> element into the DOM in this case), I want it to be updated and drawn totally separately from any of its starfield siblings (or parents or descendants).

As it is, calling the $vm.start() method (which ultimately calls MainLoop.start()) on one of the instances (where $vm = the individual starfield element I'm targeting) hijacks the MainLoop, if one exists, stopping animation on one of the starfields and starting it on another.

This is a problem for anybody who wishes to use more than one starfield per page. But it also causes issues if, for instance, two starfield elements exist briefly together in memory (as is the case when switching views with Vue). Also, since each component can have its own discrete state, having one global MainLoop feels more difficult to reason about--but that could just be me. In this particular case, it makes more sense to me for each canvas to be its own world, with its own loop.

Does that make sense?

Adding a global manager like you've written above is feasible (and maybe even preferable), but I'd have to create an entirely new Vue component (or use my parent app component) and figure out how to start/stop each instance, if desired, without starting/stopping any of the others.

Obviously having the option one way or the other might be nice, but I love how small this lib is.

@IceCreamYou

This comment has been minimized.

Copy link
Owner

IceCreamYou commented May 10, 2016

@bigsweater Thanks for the detail. This is the kind of thing I was looking for.

I think animating two separate canvases independently is potentially a good use case for having MainLoop instances, so I'm reopening this issue.

The current model for doing that would be, as you suggested, to have a delegator component that interacts directly with MainLoop and manages when each canvas gets to run its callbacks. This is fine in terms of maintaining timing accuracy and determinism, but it'd be nicer if implementing it wasn't necessary.

@IceCreamYou IceCreamYou reopened this May 10, 2016

@IceCreamYou

This comment has been minimized.

Copy link
Owner

IceCreamYou commented May 11, 2016

I also thought of another use case: libraries that want to use MainLoop whose users might also use MainLoop. Users would be likely to overwrite the handlers that the library added if they interacted with MainLoop directly.

@bigsweater

This comment has been minimized.

Copy link

bigsweater commented May 11, 2016

@IceCreamYou Exactly--if I were to distribute my starfield component as it is now, I'd have to pull MainLoop out just in case somebody else is already using it.

That said, the more I think about it, I actually love the idea of having one central manager for animation state and a single rAF loop running--it seems elegant and I imagine it would be performant (depending on the operations in the loops themselves, obviously). Having some sort of standard library like that might be nice for folks like me who don't work with animation often.

I'm using Vuex (analogous to Redux) to manage my app's state--why shouldn't there be a central animation state with an API for dealing with animation loops? It makes a lot of sense.

@jamesplease

This comment has been minimized.

Copy link
Contributor Author

jamesplease commented May 11, 2016

Yup, I agree @bigsweater. I came around after reading @IceCreamYou 's initial response to this issue (then I dropped off the face of the Earth. Sorry 'bout that :) )

I like the idea of this library remaining lightweight, as it is now, and a manager for multiple components to exist as a separate package or in userland.

@IceCreamYou

This comment has been minimized.

Copy link
Owner

IceCreamYou commented May 12, 2016

Thanks for the feedback. I agree it's important that this library remain slim. Maybe it makes sense to add an optional "callback manager" to the repo that can manage multiple loops. The pattern as a user would be something like:

var myLoop = new MainLoop.Instance();
myLoop.setUpdate(function(timestep) { /* ... */ });
myLoop.start();

When a MainLoop.Instance is instantiated it could quietly register itself with a MainLoop.Manager. If there are no instance loops previously registered, the manager could then set itself up with the core MainLoop to execute its instance loops' callbacks when needed. When #start() is called on an instance, it could just internally ask the manager to make sure the core MainLoop is running and then flip a boolean that the manager would use to determine whether to run its callbacks.

Thoughts?

@jamesplease

This comment has been minimized.

Copy link
Contributor Author

jamesplease commented May 12, 2016

That sounds like a solid API to me, @IceCreamYou .

@bigsweater

This comment has been minimized.

Copy link

bigsweater commented May 12, 2016

@IceCreamYou I'm still learning JS and don't know too much about architecting a system like this, but that sounds like it would be a winner. Just as easy to use as the current library.

@IceCreamYou

This comment has been minimized.

Copy link
Owner

IceCreamYou commented May 16, 2016

Good. I'm not sure when I'll be able to get to this, so if anyone feels like providing a PR, that would move things along 😃

@bigsweater

This comment has been minimized.

Copy link

bigsweater commented May 18, 2016

@IceCreamYou I'm buried but hopefully my site will launch next week! After then I want to take a crack at it.

@iwilliams

This comment has been minimized.

Copy link

iwilliams commented Jun 28, 2016

I came here because I thought I had a use case, but as I type up my questions and ideas I find myself questioning the need for multiple loops....

My use case would be to run multiple simulations in a single node process for a multiplayer game, where each "server" would get its own loop. I was originally thinking that each server would need its own loop because you wouldn't want simulation ticks to slow down other people's games, but if all the loops are in the same process the time to process a tick is going to block the other loops if it takes too long anyway. Now what I'm thinking is in each update call I would just loop through all active instances and advance game logic for each one.

I think that my use case is fundamentally similar to @bigsweater, but I find myself coming full circle to the original answer by @IceCreamYou. It seems to me that updating isolated states would be better managed in registered update function which I think is kind of the idea with the most recently proposed "callback manager" solution.

I guess I don't really know where I'm going with this, but I wanted to get these ideas out there since I was considering doing the refactor to support multiple instances.

@IceCreamYou

This comment has been minimized.

Copy link
Owner

IceCreamYou commented Sep 5, 2016

Okay, I wrote some code to do what was discussed above: https://gist.github.com/IceCreamYou/6b16d8350bc4125a08c76b499e0d4c59

I'm interested in some opinions:

  • Should a variation on this code be part of the main MainLoop.js file? It adds something like 500 bytes minified and gzipped (for reference, MainLoop.js is currently ~750 bytes minified and gzipped).
  • Otherwise, should this code be included as a second file in this project? This has several advantages: including it is optional, the default of not having it is lower risk, and the primary source file remains easier to understand. However, it has the disadvantage that the code needs to know how to find the MainLoop object, and unfortunately it can't do that if a module loader is in use, so it requires the MainLoop object to be passed explicitly in that case.
  • Alternatively, should this code be available as a separate project?

In addition to answering these questions, the code also needs testing (e.g. by extending the demo) and an update to the README before it should be published to npm.

@jamesplease

This comment has been minimized.

Copy link
Contributor Author

jamesplease commented Sep 5, 2016

I also thought of another use case: libraries that want to use MainLoop whose users might also use MainLoop. Users would be likely to overwrite the handlers that the library added if they interacted with MainLoop directly

For the record, this would only affect two users who are using the global version of the module, which I would guess is a smaller number (tho maybe folks in the gaming community do this more than in the app community). If you're using CJS/AMD/ES2015 modules then you'll get a new MainLoop when you import it.

The proposed problem could be further mitigated if MainLoop wasn't attached to the root object by default. It should only be attached to the root if a module loader isn't detected imo.

Then, there's no likelihood of collision if you're using a module system (which most folks should be doing these days; especially if they're packaging JS to be distributed on other sites). And within each project it makes the most sense to use 1 MainLoop I think.

The Vue example provided above gives a good reason to still think about resolving the problem, though :)


@IceCreamYou , I typically take on a burden of a more complex build system in my libraries to provide the best API for the consumer. In this case, if I wanted to distribute two versions, then I would use a build system to dynamically generate two files. That way, the consumer wouldn't need to worry about passing in the MainLoop object to the constructor.

With that said, I usually make that decision based on the size tradeoff. In my opinion, 500 bytes added to 750 bytes isn't enough to justify that, even though from a % perspective it's a big jump. Others might feel differently, though, and that's an understandable perspective, too. As an aside, did you factor in the fact that you've included the UMD wrapper in the new code?

I think the API seems fine, but I don't think it's the best possible API for this lib. There are three things that stand out to me:

  1. MainLoop.Instance is a slightly unusual naming pattern, I think. A less unusual pattern would be: MainLoop is the constructor, mainLoop is the default instance.
  2. there is a constructor and an initial export that appears to be an instance of the constructor, but isn't. If there's some way to make the initial export just a new MainLoop() without any performance consequences?
  3. If the build system were updated to export ES2015 modules (it would be backwards compat), it could be pretty elegant. Maybe an issue for another day, though :)
// Pull in an already-created instance
import mainLoop from 'mainloop.js';

// Pull in the constructor
import {MainLoop} from 'mainloop.js';

// Use the constructor
const myLoop = new MainLoop();
@IceCreamYou

This comment has been minimized.

Copy link
Owner

IceCreamYou commented Sep 6, 2016

Thanks for the feedback.

If you're using CJS/AMD/ES2015 modules then you'll get a new MainLoop when you import it.

This is incorrect. Every module loader I know of caches state the first time a module is loaded. Otherwise, singletons wouldn't be possible. If you need proof, try this:

$ echo "module.exports = {};" > a.js
$ echo "var a = require('./a.js'); a.b = 3;" > b.js
$ echo "var a = require('./a.js'); require('./b.js'); console.log(a);" > c.js
$ node c.js
  { b: 3 }

The proposed problem could be further mitigated if MainLoop wasn't attached to the root object by default. It should only be attached to the root if a module loader isn't detected imo.

Indeed, this is how it already works.

MainLoop.Instance is a slightly unusual naming pattern, I think.

That is not the interface made available by the code I posted. The interface is:

// Without a module loader
var loop = MainLoop.createInstance();

// With a module loader
import {MainLoop} from 'mainloop.js';
import {createMainLoopInstance} from 'mainloop-multiple.js';
var loop = createMainLoopInstance(null, MainLoop);

There is a more detailed example in the code which also covers mixins. In any case, the gist was just the simplest thing that worked in external code. Since MainLoop is currently an object, it's not possible to just create instances of it without modifying the library. It could be imported and re-exported, but that requires some knowledge about the client's module loading system. If I end up including the code directly in the library I will probably make MainLoop a constructor. However, as addressed in this thread and in the gist, there are downsides to making multiple loops the default.

If the build system were updated to export ES2015 modules (it would be backwards compat), it could be pretty elegant.

This library is already backwards- and forwards-compatible by supporting AMD, CommonJS, and global objects. There are two advantages of ES2015 modules:

  • It's easier to write the word export than to copy-paste 5 lines of boilerplate UMD. This isn't an issue here because the library is already written.
  • It is possible to produce smaller minified builds with a tree-shaking compiler like Rollup because Rollup can more easily detect which parts of the code aren't used in the client and eliminate them from the combined bundle. Useful, but not a huge advantage for a library that is already < 1 KB.

If I were to update the code to use a newer JavaScript dialect, I'd pick TypeScript, but I'd do it for the type checking more than anything else. I'm already using this library in a TypeScript project with the typings I added a couple weeks ago and it's pretty nice. I'd still compile down to UMD but such a setup would allow compiling to ES2015+ as well.

@jamesplease

This comment has been minimized.

Copy link
Contributor Author

jamesplease commented Sep 6, 2016

This is incorrect. Every module loader I know of caches state the first time a module is loaded. Otherwise, singletons wouldn't be possible

You're right about this, and I didn't mean to convey that I thought it worked any differently. The example I was thinking of was embedding a MainLoop bundled app on another page with another bundle. As long as neither reference the global version (which would require that they're using some sort of module loader), then I'd expect that it would reference a different MainLoop.

I was being silly, though, and not thinking about pulling in, say, a third party Vue or React component that does import the same MainLoop, which is more likely to be the common use case. So it probably wasn't a point worth making :)

Indeed, this is how it already works.

I don't have my computer to check (typing on my iPad atm), but a quick glance looks like it's always defining MainLoop on the root obj:

root.MainLoop = {

It looks like this would set global.MainLoop in node, and window.MainLoop in the browser, always. My suggestion to mitigate the possible issue I mentioned would be to only do this when CJS/AMD aren't detected. This way, if you've got two bundles on the same page then it would be impossible for them to reference the same MainLoop. If they used window.MainLoop, even in a CJS bundle, then they could be using another bundle's MainLoop. Again, this is an edge case shrug

Re: everything else –– sounds good!

The two benefits of ES2015 modules you mentioned are absolutely true. The thing I was emphasizing was that the named exports + default exports can provide a nice syntax for exporting an instance in addition to a constructor. CJS/AMD can accomplish the same thing, but with less nice syntax, I think, given that both must be on the same obj rather than separate things. It's really not a big deal, though.

Anyway, most of the things I mentioned could probably be brought up in other issues, if they're even worth thinking about at all. The Gist looks good. Nice work ✌️

@IceCreamYou

This comment has been minimized.

Copy link
Owner

IceCreamYou commented Sep 6, 2016

The example I was thinking of was embedding a MainLoop bundled app on another page with another bundle. As long as neither reference the global version (which would require that they're using some sort of module loader), then I'd expect that it would reference a different MainLoop.

I guess if you included MainLoop on the same page twice and you were able to capture each load separately then this would be the case. That would be marginally less optimal though because the browser only has one render loop but multiple MainLoops would be competing for its attention. It would be difficult to predict each loop's time budget and which loop would run first in any given frame.

I don't have my computer to check (typing on my iPad atm), but a quick glance looks like it's always defining MainLoop on the root obj

Ah, you're right about this with browser-based module loaders that don't load modules in a custom context, which is true at least for SystemJS and probably for Require.js. In Node and Browserify, the root object is an alias for module.exports. I'm not sure about other loaders.

@bigsweater

This comment has been minimized.

Copy link

bigsweater commented Oct 4, 2016

Apologies for the late reply!

I haven't tested your new code, but thanks for putting it out there. I'll give it a try as soon as I can.

I'm partial to the idea of breaking it out into a separate, optional file; as long as it's clear in the docs that that the object must be passed in to hook into a single instance, I can't imagine it being a huge problem.


I know this is a totally different architecture than what you've built here, and I'm by no means requesting this sort of functionality, but I figured it couldn't hurt to mention:

I was going to mimic the functionality of fastdom--where there's a single global instance of fastdom, and you push work to the fastdom queues (arrays that are cleared out every iteration of the rAF loop).

I'd like to try my hand at that implementation. I'm not totally clear on the advantages/disadvantages of either approach (instantiation vs. a queue), but as far as I can tell, a queue system might allow for:

  • Minimal overhead (a few new methods to add work to a given queue, and a queue processing method)
  • One instance, 'multiple' loops
  • It's explicitly global; multiple modules from multiple bundles all use the same window object

This may not be the best way to handle it, obviously. Just throwing it out there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.