Add helpful message about pooled classes

[sophiebits]

Hopefully this will make fewer people confused at all of the null properties. The string won't wrap nicely so I tried to keep it pithy:

Better suggestions appreciated.

[chenglou]

You could highjack the class's toString and log a message there, but this changes the behaviour between dev and prod.

[syranide]

@chenglou The issue I see with overloading toString is that we break it for browsers that don't delay evaluation like Chrome... unless we dynamically add/remove it, which could actually make a lot of sense in dev, but I'm assuming Chrome wouldn't use toString unless it was there when it was logged to the console?

@spicyj Should be cleared when persist() is called I would think? Also, the way it's done currently, you're setting _ABOUT_POOLING even in prod, which seems less than ideal. Perhaps Proxy could be used in dev to implement this properly? (Support is really lacking or even non-existant currently perhaps?)

Althought it feels kind of icky to modify the object like this. toString would seem like the neatest solution overall, but it would probably not behave correctly Chrome as I mention above?

[sophiebits]

@syranide It's only set in the destructor which isn't called if .persist() is. I've intentionally created the property even in prod; we try hard to avoid any observable behavioral differences between dev and prod and you could perhaps loop through the properties of an event and somehow get confused.

[zpao]

👍 Anything you want to change about it @yungsters?

[yungsters]

This is pretty cool, looks good to me.

[zpao]

Of course @sebmarkbage wanted some changes right after I merged. (something about deopting hidden classes)

[yungsters]

Well... for what it's worth, we only de-optimize the pooled objects once until the pool gets warm.

[sophiebits]

Tips for reducing the hidden class impact here? I made an attempt to set it as soon as possible – does it need to be in the constructor instead?

[sebmarkbage]

It might be possible to add it somewhere else.

The problem is that I want to get rid of this pattern of adding additional properties to instances because it becomes incredibly difficult to reason about statically and around performance. It doesn't matter if we can find a case where it wouldn't deopt because it's still difficult to reason about and it still adds memory overhead in PROD.

You could potentially use something like displayName or valueOf on the prototype to name Objects. It's already called SyntheticEvent. You can probably get that to be "SynthenticEvent - Pooled class whatever..."

[sophiebits]

Reverted in 92d2dcc.

[syranide]

@sebmarkbage A curious question, do we really need pooled classes for events, can there ever be more than one of the same event type in flight at any one time?

[zpao]

I vaguely remember us having this discussion some where else, but I think the reasoning is that events get created a lot. If we create new SyntheticEvents all the time, I think we're going to hit GC much more frequently.

[sebmarkbage]

I think what @syranide is asking is if it can just be a single global instead of pooled. Possibly. They may have different signatures though.

On Jul 12, 2014, at 5:24 PM, Paul O’Shannessy notifications@github.com wrote:

I vaguely remember us having this discussion some where else, but I think the reasoning is that events get created a lot. If we create new SyntheticEvents all the time, I think we're going to hit GC much more frequently.

—
Reply to this email directly or view it on GitHub.

[syranide]

@sebmarkbage Indeed. Practically, could we replace the use of PooledClass for Synthetic*Event with say ReusedClass, that instead only holds a single reference (rather than an array). Rather off-topic, but it just came to mind when I saw this issue.

[sophiebits]

If you call node.focus() then focus handlers are triggered immediately, so I believe you can have two focus events in flight at the same time.

[syranide]

@spicyj Imagineable (and interesting!) and I'm not arguing for there being a lot of practical merit to my idea, but I imagine that for your scenario, spawning an additional "non-pooled" event is a non-issue, unless that is true for other events too.