Marcus' random performance notes

[marcustyphoon]

I'm not sure exactly when I'll get to looking at this list again, but I did make it, so... here's the mostly-finished notes I took while skimming random portions of the XKit Rewritten source a little while ago! In short, I didn't find anything to noticeably improve, which isn't surprising for a carefully thought out vanilla javascript app.

Potentially worthwhile performance improvements:

• coalesce timestamp insertion (either by parsing post IDs for them and applying them synchronously, or batching them with a debounce or Promise.all)
• append dom element after finishing setting its properties (mostly in timestamps, other places I don't think it matters much)
• exposetimelines: skip inject() call if all timelines exposed already
• notificationblock: get notificationSelector outside of callback

Probably meaningless, but somewhat interesting/haven't looked yet:

• react_props cache could be a Map (how to test impact of this? idk)
• tag tracking plus: promise.all the apifetches? could increase delay though
• todo: look at meatballs.js

Certainly meaningless:

• investigate: can mutual checker/painter use timelineobjectmemoized without bugs? probably not?

• quick tags processPosts: probably better non async

• NotificationBlock: processNotifications could be called only when unprocessed notifications exist, using an excludeclass, replacing an injected function with a QSA whenever there is a page mutation. Saves 1.3ms (of hundreds).

• NotificationBlock: I bet the loop in processNotifications can be synchronous, which could save a ~single style recalc in theory maybe possibly.

• Querying local storage in processPosts instead of keeping a local state adds about ~1ms (I think) of async delay, but, whatever

• painter: cache api call for source tags? would only be a perf gain if you refresh dashboard lol

• cssMap gets called 3-4 times before the first resolution sets the cache,

Not a performance gain, but I put other notes here:

• investigate making injected scripts named functions for profiling
• todo: check error messages for clarity
• quick tags: difference between mega editor api and this way? potential not to break markdown posts? (is this still a thing?)

[AprilSylph]

can mutual checker/painter use timelineobjectmemoized without bugs? probably not?

Painter definitely can't, since one of the conditions is "is this post liked", which is very volatile

cssMap gets called 3-4 times before the first resolution sets the cache,

I feel like I could fix that one!

investigate making injected scripts named functions for profiling

I believe this one is entirely possible since Function.prototype.toString() retains the name, at least for a function declared inline, which is how we use inject() anyway across the entire codebase (to my knowledge)

quick tags: difference between mega editor api and this way?

Mega Editor endpoints treat tags case-insensitively, meaning it would break adding "#CATS" to a post already tagged "#cats" (not sure why you'd want to do this, but it would be a weird inconsistency since the post editors on the dashboard also allow this)

potential not to break markdown posts? (is this still a thing?)

nope! this is the new dashboard, so we only deal with posts' NPF representations. even if the beta post editor gets a markdown mode, posts will be saved and served to us as NPF.

[marcustyphoon]

Painter definitely can't, since one of the conditions is "is this post liked", which is very volatile

...Now that you mention it, does this actually work? Obviously the colors being correct only after you change pages is better than it being wrong until you hard refresh, so yeah, unmemoized is still better, but I assume there's no live updating of the colors. Would be neat though.

making injected scripts named functions

Yep, works great. I have a branch with most of the async callbacks named, now - nice for profiling. Not sure if we want to do so for real? It feels a bit inelegant compared to arrows.

[marcustyphoon]

All of the above notes were in the "fun if you like to use micro-optimization as a way to learn about Javascript, but insignificant to the user" category. Scrolling down the dash, though, results in some actually perceptible delays that are theoretically improvable.

We're still talking "wouldn't really add to the user experience and would take development time and make code harder to understand, test, and maintain (unless there's some cleverness I haven't yet thought of)" so I don't know if these are necessarily worthwhile, either. Still fun to think about and study though!

Nothing even vaguely close to, say, the XKit 7 postblock recursive exponential infinite loop scenario.

After loading 1 day of my timeline (250 posts, or 130k DOM elements)...

querySelectorAlls and querySelectors get rather expensive. (~17ms each, or a little under 10ms if you only look for a class; upwards of 800ms for an infinite scroll load, though this is isn't approaching the time spent by the Tumblr code itself). These are about half from timelineObject calls (fired twice, I think, with Painter and Mutual Checker both enabled.)

Improvements could include:

• Reusing them (one cheap QSA + a loop over all post elements that checks for every excludeclass at once looks about as fast as a single fancy QSA) - this could require invalidating a cache when onNewPosts fires, coalescing synchronous processPosts calls to use the same call, or other trickery. Could be an argument to the callbacks passed to mutations.js? (Easy to implement; very unintuitive to use).
• Getting postElements directly from their parent node instead of via QSA? Assuming all posts are children of one element, querying 130k nodes instead of like 300 is a bit silly. Element.children? (have not tested this yet; I tried element.QSA and it's worse)

Both of those kind of apply to timelineObject, too, though they seem much harder to do in that case, since timelineObject calls don't interact with each other. On that note:

Injected functions get rather expensive (well, their callbacks do: 40ms each, adding to a few hundred). Apparently Tumblr fires a ton of handlers on MessageEvents. (Looks like it might be partially or wholly Sentry? Anyway.) I dunno if there's a more efficient way to access the page context from a content script, but I didn't see one from some googling.

Improvements could include:

• Batch multiple apiFetch calls and/or multiple timelineObject calls into one inject call with a single callback via Promise.all? Like above, this could require trickery with timers, a change to the processPosts QSA forEach pattern, integrating timelineObject into getPostElements, etc.
• Run a persistent script on the page so it can have more logic? Can you use MessageChannels between page context and content script? Page script could combine multiple calls into one callback? Page script could contain the source for all the injectable functions so the eval() would only happen on first load?

Edit: Hah, the second one is almost certainly Sentry. They replace window.addEventListener with their own custom function (try typing window.addEventListener into the javascript console!): https://github.com/getsentry/sentry-javascript/blob/295d26312219b978acadafeccec6b73754097dea/packages/utils/src/instrument.ts#L520-L533

I think there's a bug in their code because the number of event listeners on the page just keeps going up as you scroll.

[AprilSylph]

Could be an argument to the callbacks passed to mutations.js? (Easy to implement; very unintuitive to use).

(Assuming I'm thinking of the same thing you are) I can vaguely see this being feasible. The callback functions registered to onNewPosts etc. are supposed to basically be custom events, so why not add custom arguments too? This might actually help me, as I've been wanting to make getPostElements() async. All the different arguments for getPostElements() could simply be implemented into a filter function.

Can you use MessageChannels between page context and content script?

I don't see anything suggesting you can't, so I guess we could try it? Would be very exciting if it works!

[marcustyphoon]

(Assuming I'm thinking of the same thing you are)

My guess is that we are, as well, though I'm not 100% sure! (You know the framework much better than I do, of course, so it's also possible that what I was thinking of is a vague broad category of things, some of which don't actually make sense, and you're thinking of is the correct interpretation of that :D

This might actually help me, as I've been wanting to make getPostElements() async.

I'm really curious what this means, but I don't have any interpretations of it that immediately make sense off the top of my head!

[AprilSylph]

I guess getPostElements() would just disappear. What I want to do is combine it into the MutationObserver in mutations.js, so that new posts can get passed directly to all the registered callbacks as an array argument. Thinking about it more though, this wouldn't introduce any asynchronisity since there wouldn't be any filtering done there; instead, the callbacks would feed the received argument into a function that takes all the same arguments as getPostElements() does now, which would be async so that it could call exposeTimelines() where needed automatically.

[AprilSylph]

Looks like MessageChannel works just fine! The code is a bit messier than I'd like since ESLint won't let us use top-level await so you might need to make your own adjustments to make it more profileable again, but this means completely avoiding Tumblr's own message event listeners! Woo! (I'll commit it shortly, gotta run some more tests first.)

[marcustyphoon]

Just a note here for posterity: while Sentry does do some interesting things, replacing native window functions with wrapped versions that it uses to record the last few things that happened in the case of a crash, the thing that appears to cause Tumblr to fire so many handlers on messages is that they're used to communicate ad data to all the ad containers on the page, which naturally increase in number as you scroll.