Node history markers require an HTTP request

just going to mention that all the fetched nids, whether pre-loaded or pulled in via a HTTP request "on demand" are cached in localStorage.

For users with hundreds of nodes in their history that are "new", there probably needs to be a cap on how many can be pulled into localStorage, or considered "new" at all, regardless of what we do to avoid extra requests.

#1: that's a good additional heuristic, except that it will almost always exceed the "30 days" mark, beyond which we always just mark nodes/comments as read.

I still don't see how prefetching NIDs "last read timestamps for the X most recently read nodes" is going to have a big impact; it may have an impact, but it might just as well not. It'd be much better to just know which nodes are on the current page (and as said in #1991684-40: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user, we already know that!) and then just #attach all necessary "last read timestamps" data in such a way that it won't end up in the render cache. I.e. the #uncached_attached that I was talking about.

No, attaching the NID and whatnot is pointless, it's not the problem that we need to solve. The #cache array is built for each page load anyway, in order to calculate the cache key!

The problem is: if you want to embed the current user's last read time stamp, it must happen through drupalSettings and to do that you must use #attached, and if you do that, then it will end up in the render cache!

(I agree with the overall concept, but that's also what I've been saying; the problem is that with our current infrastructure we can't do it, hence my proposal for something like #uncached_attached.)

I'd be interested in a formal proposal for more fine-grained cache settings for #attached (why just cached and uncached? why not support page/user/role granularities too?) that is well thought out and has some proof of concept patches. I am a little bit concerned that there's only one use-case (AFAIK) for the addition, but I'm sure we could find more examples if we wanted to look.

That said, the pre-fetching/cache warming approach is a potential fix for the exact problem with node history markers outlined here and in other issues and it doesn't require API modifications/additions, so in the end it might just be easier to get over the line for D8.

#6: the reason cached vs. uncached makes sense is because it's the simplest possible thing that could work: when uncached, use logic to determine what should be attached.

If you're going to define "this is attached for roles X and Y", then … indeed you're entering tricky territory, because it's nigh impossible to anticipate all possible contexts. Just "uncached" is free of context.

"Prefetching the data for the most recently read X nodes" is flawed, as I've explained numerous times by now, in this issue and in the other. Cache warming is something else, and that'd be even more flawed, because for some things you will inherently have to do requests because you can't be certain that the client-side cached data is the latest. So the benefit would be negligible.
Embedding the metadata on the page rather than doing HTTP requests is only going to work if you embed all the required data. Otherwise, you're still going to end up doing HTTP requests for the exact same scenarios that #1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user is already making requests for. The only benefit would be to not have a HTTP request to "warm the cache", after that it'd be exactly the same as what's already being done in that issue.

You may want to read #1991684-40: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user, thedavidmeister.

reading now :)

sorry, I didn't read anything. I got hijacked in IRC. I will go over this again as soon as I get the chance.

#10: How would you implement that cleanly? Could you implement that cleanly with the APIs we have today? I don't yet see how.

#1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user landed, so we can now continue here.

If we going to have #1029708: History table for any entity then improvement should affect all entities so probably better to wrap history logic into #2081585: [META] Modernize the History module so allow contrib to easily swap the service with more performant implementation that could be specific to target site.

The HISTORY_READ_LIMIT also should be defined as class constant to be overridable

This is now blocked on #2118703: Introduce #post_render_cache callback to allow for personalization without breaking the render cache. The discussion about potential solutions that we had in this issue in comments #1–#11 is incorporated there.

Once that is in, this patch solves this issue.

#post_render_cache/#2118703: Introduce #post_render_cache callback to allow for personalization without breaking the render cache landed a while ago. Several big performance patches already leveraged it. Now let's do this one :)

While rerolling, I found a bug in the above patches: the comment ID instead of the node ID was being used to retrieve the last read timestamp.

And I also found a subtle bug for #post_render_cache usage, despite the battery of tests that was added by #2118703: #post_render_cache callbacks for child elements are only processed when #cache is set. This broke the attaching of the last read timestamp as drupalSettings on individual comments: the callback would never be called.

Since this gets rid of one HTTP request per page — on many pages, for all authenticated users on most sites, this is at least major, probably critical.

+++ b/core/modules/comment/comment.module
@@ -1731,3 +1739,54 @@ function comment_library_info() {
+function comment_attach_new_comments_link_metadata(array $element, array $context) {

My only observation is perhaps this could be a static method on the CommentViewBuilder object for the sake of lazy loading (ie instead of having it in .module and loaded all the time).

#17: good point. Done.

I was tempted to do the same for history_attach_timestamp(), but it feels wrong to have a public static attachTimestamp() method on HistoryController. What do you think?

18: history_personalization_http_reqs-2073535-18.patch queued for re-testing.

I think we are good here.

#23: each "thing" on the page declares which #post_render_cache callbacks it wants, with which context. To be able to use one history_read_multiple() call for *all* the "things" on the page that say they want some history read, we need to make the code responsible for invoking #post_render_cache callbacks smart enough to be able to merge multiple contexts together. That is not trivial.

It might be possible, but it would require a significant change in how #post_render_cache works, and might require additional metadata to be declared ("is mergeable"). Therefor, that is significantly out of scope.

If you think it's crucial, then we should probably postpone this issue on getting support for that in #post_render_cache. Your call. Back to RTBC for now.