🇩🇪Germany @Fabianx

Account created on 12 January 2010, about 14 years ago
#

Recent comments

🇩🇪Germany Fabianx

Just to re-iterate that point:

- Automatic enabling is good for 80% of sites, which do not care either way
- Being able to opt-out of automatic enabling is important for sites, who want to control the process of what gets installed when

It is important to not just take the stance that "you can disable it afterwards again", because of privacy concerns when the feed is consumed, which can happen when e.g. cron runs in between module enabling and disabling.

🇩🇪Germany Fabianx

I agree. Let's add some constants, which are used in both places.

🇩🇪Germany Fabianx

Hello there!

At Tag1 we have been hard at work to create a drop-in replacement for the Layout Builder UI, which is fully compatible with all existing contrib modules and used by our Enterprise clients already for a while in production.

It essentially changes the experience in three key points:

- a) 1 drag instead of 5 clicks (with full best-effort configurable auto-completion of blocks using sample images and lorem ipsum ...)
- b) Create your layout automatically -> Just drag your blocks and then change the auto generated layout to fit your needs
- c) BETA: Nested layouts including the ability to drag blocks into a sub-layout (think of it as Layout UI for blocks itself)

It's called lb_plus and can be found here including a small GIF to show the awesomeness:

https://www.drupal.org/project/lb_plus →

Some more detailed description by Tim Bozeman, who wrote the second version of lb_plus follows:

Like larowlan said, we've been working on a drop in replacement for the Layout Builder UI that addresses many of these issues. There has been interest in adding LB+ to core also.

lauriii said:

One idea was to build a community "recipe" that installs targeted contributed modules on top of Layout Builder, to have a more opinionated starting point.

I think LB+ can help fill the gap.


Features include:

  • Drag and drop block placement
  • See what blocks do at a glance with block icons
  • Place blocks quickly with Promoted Blocks
  • Sortable sections
  • Change a sections layout
  • Duplicate blocks
  • A cleaner UI that looks more like the published content while editing
  • Soon to be released: Nested layouts ✨ Support Inline Layout Blocks Fixed which adds a whole new block builder functionality

Click here to see LB+ in action!

🇩🇪Germany Fabianx

Back to RTBC!

One thing is that we should open a follow up issue to generally track cache events in the caching layer and not in the database layer.

As the performance data is neutral to where it comes from , this is straightforward:

- Collect cache events
- Subscribe to it
- Add to performance data
- Ignore DB cache things instead

🇩🇪Germany Fabianx

Approved!

I am fine with that change.

I agree with the great review, but think that the additional columns should not make a problem in practice.

🇩🇪Germany Fabianx

RTBC - Code looks good to me, fixes an important oversight from the last MR related to this and overall enhances the performance testing capabilities.

Only caveat is that only pages loaded via the Mink driver can be performance tested in that way right now, but that is by design and hence the perfect scope and hence fine for this issue.

🇩🇪Germany Fabianx

#9: If this is false, what should happen? Do we assume it got set with the correct value already and silently skip it?

If this is false, then another process has gotten the "lock" and either calls setWithoutLock() right now, has called setWithoutLock() already or will call setWithoutLock() in the near future.

As this is just about avoiding write stampedes to the same item (first step), there is likely even a process that is still before the apcu_fetch() and will write the data again, so we also don't need to worry about the process that got the "lock" to crash.

Someone else will then set it.

🇩🇪Germany Fabianx

Here is the ChatGPT analysis of this function:

You’ve highlighted an important aspect of how these APCu functions interact with the underlying data store, and you are correct in your understanding of the different lock mechanisms they employ.

### apcu_cas()

- `apcu_cas()` operates with a lighter locking mechanism.
- It only requires a reader lock on the entry.
- It performs a compare-and-swap (CAS) operation, which is atomic.
- This means that it can check the current value and, if it matches the expected value, update it in a single atomic operation.
- Because it only needs a reader lock, it doesn't block other reads from happening simultaneously.

### apcu_store()

- On the other hand, `apcu_store()` requires a writer lock.
- A writer lock is more restrictive; it blocks both other writers and readers until the operation is complete.
- This can lead to longer wait times and potential bottlenecks, especially under high load or when dealing with large data sets.

### Why Your Approach Is Beneficial

- By initializing the lock with `apcu_add()` (which uses `apcu_store()` internally but is only called once at the start), and then using `apcu_cas()` for subsequent lock acquisitions, you ensure that the heavier writer lock is acquired less frequently.
- Most of the synchronization relies on `apcu_cas()`, which uses the lighter reader lock, resulting in less contention and better performance.
- This is especially beneficial in a high-concurrency environment, where many processes are trying to acquire the lock simultaneously.

By understanding and leveraging the underlying mechanisms of these APCu functions, your approach efficiently reduces lock contention, leading to better performance and scalability. This kind of optimization is crucial in performance-critical applications and high-load scenarios.

As background reading.

🇩🇪Germany Fabianx

I think this is independently useful as it replaces one write in 99% of concurrent cases with two reads and just one non-concurrent write plus one additional cache key.

🇩🇪Germany Fabianx

Here is another optimization of it:


public function set($cid, $data, $expire = CacheBackendInterface::CACHE_PERMANENT, array $tags = []) {
  $key = $this->getApcuKey($cid);
  $lock_key = $key . ':apcu_update_lock';

  $results = apcu_fetch([$lock_key]);
  $old_lock_value = $results[$lock_key] ?? FALSE;

  // Ensure lock key exists.
  if ($old_lock_value === FALSE) {
    // Initialize the lock with a random value using apcu_add().
    $old_lock_value = mt_rand(0, 2147483647);
    apcu_add($lock_key, $old_lock_value);
  }

  $lock_value = mt_rand(0, 2147483647);
  if (apcu_cas($lock_key, $old_lock_value, $lock_value)) {
    // Lock acquired, you can perform your update here.
    parent::set($cid, $data, $expire, $tags);
  }
}

Here is the ChatGPT analysis of this function:

You’ve highlighted an important aspect of how these APCu functions interact with the underlying data store, and you are correct in your understanding of the different lock mechanisms they employ.

### apcu_cas()

- `apcu_cas()` operates with a lighter locking mechanism.
- It only requires a reader lock on the entry.
- It performs a compare-and-swap (CAS) operation, which is atomic.
- This means that it can check the current value and, if it matches the expected value, update it in a single atomic operation.
- Because it only needs a reader lock, it doesn't block other reads from happening simultaneously.

### apcu_store()

- On the other hand, `apcu_store()` requires a writer lock.
- A writer lock is more restrictive; it blocks both other writers and readers until the operation is complete.
- This can lead to longer wait times and potential bottlenecks, especially under high load or when dealing with large data sets.

### Why Your Approach Is Beneficial

- By initializing the lock with `apcu_add()` (which uses `apcu_store()` internally but is only called once at the start), and then using `apcu_cas()` for subsequent lock acquisitions, you ensure that the heavier writer lock is acquired less frequently.
- Most of the synchronization relies on `apcu_cas()`, which uses the lighter reader lock, resulting in less contention and better performance.
- This is especially beneficial in a high-concurrency environment, where many processes are trying to acquire the lock simultaneously.

By understanding and leveraging the underlying mechanisms of these APCu functions, your approach efficiently reduces lock contention, leading to better performance and scalability. This kind of optimization is crucial in performance-critical applications and high-load scenarios.

As background reading.

🇩🇪Germany Fabianx

Here is the code for set() using just the lock mechanism:

public function set($cid, $data, $expire = CacheBackendInterface::CACHE_PERMANENT, array $tags = []) {
  $key = $this->getApcuKey($cid);
  $lock_key = $key . ':apcu_update_lock';

  $results = apcu_fetch([$lock_key]);
  $old_lock_value = $results[$lock_key] ?? FALSE;

  // Ensure lock key exists.
  if ($old_lock_value === FALSE) {
    apcu_add($lock_key, 0);
    $old_lock_value = 0;
  }

  $lock_value = mt_rand(0, 2147483647);
  if (apcu_cas($lock_key, $old_lock_value, $lock_value)) {
    parent::set($cid, $data, $expire, $tags);
  }
}

This however brings to mind the question why APCu itself could not protect it's apcu_store() mechanism in this way.

But it would take quite some time to get it into APCu, so still useful here.

🇩🇪Germany Fabianx

Last to explain the lock:

The lock is for ensuring that only one item ever is written at the same time to the same cache key.

This can again be independently done:

It works like this:

- Retrieve the old value of the lock entry
- Create random value
- Do a CAS on the old value (compare-and-swap)

If we win the race to swap, then we will write the new entry.

Note: There can still be several items written after each other, but at least not concurrently.

Again the check for if the data is still the same, can help here.

🇩🇪Germany Fabianx

An optional optimization that the IS patch had done was also:

- Because we already retrieved the items from the fast backend, we can continue to make use of it.

No need to re-get the timestamp and cache_item when we just did the same before.

This is why ChainedFastBackend.php was patched to have an additional argument to the ->set().

Probably better if this is explicit to have a different set() method for when items are given that had just been retrieved from cache - if we wanted to do that.

🇩🇪Germany Fabianx

Let me quickly write up the idea behind the code:

When a cache entry is retrieved from primary cache backend and it has not changed in the fast backend, then all we do is write the invalidation timestamp again and again.

We also might have several processes at once writing to the same item - again: the same data -- except for the timestamp. While before, the conflict would be resolved naturally by: last writer wins, now the conflict is resolved using an apcu lock.

This lock is the second part of the patch, but should probably be a follow-up as it makes an already non-trivial architecture harder to understand.

Given these properties we can split up each cache item into two parts:

- The data to be written
- The timestamp of the cache item

By splitting it up, we can "store" the new timestamp using a compare-and-swap operation without having to write the whole item.

So what we do on a set() from the chained fast backend:

- Retrieve items from APCu: [$old_cache_item, $timestamp] = apcu_fetch(...)
- Compare if $old_cache_item->data === $data [and probably same for tags now]

If it is not the same -> write: $cache_item normally [todo: locking code for sets]

compare-and-swap the new timestamp in, because we know the data is correct right now

- On gets:

[$cache_item, $timestamp] = apcu_fetch(...)

Overwrite the timestamp in $cache_item with the $timestamp from APCu - unless it's 0.

🇩🇪Germany Fabianx

While we never got #94 in, it also has other problems.

That said I think we can fix the remaining edge cases, by doing this:

diff --git a/includes/module.inc b/includes/module.inc
index 494924f..24e5dd7 100644
--- a/includes/module.inc
+++ b/includes/module.inc
@@ -833,6 +833,11 @@ function module_hook_info() {
  * @see module_implements()
  */
 function module_implements_write_cache() {
+  // The list of implementations includes vital modules only before full
+  // bootstrap, so do not write cache if we are not fully bootstrapped yet.
+  if (drupal_static("drupal_bootstrap_finished") != TRUE {
+    return;
+  }

and then set the drupal_static at the end of the bootstrap full function.

That way - that at phase is set when it's entered, not when it's completed - would no longer bite us here.

🇩🇪Germany Fabianx

Back to RTBC! Changes look great to me!

Nice API!

🇩🇪Germany Fabianx

Oh - I love that idea of just storing the returnValue in the performanceData.

++ to less boilerplate.

if ($return) {
  $performance_data->setReturnValue($return);
}

$return = $performance_data->getReturnValue();

both feels really good.

And given it's an edge case that probably is not needed in 90% of cases, it's good to optimize for the common case.

🇩🇪Germany Fabianx

Re-reviewed and the changes look good to me.

Originally I thought that the collectPerformanceData() could just return the $performance_data object, but as it also needs to return the result of the inner closure, passing it as an argument is the best solution indeed.

I agree that a trait is the best compromise between an inflexible class and a hard-to-register MiddleWare.

A MiddleWare can easily be created in the future to test normal page requests using the new trait.

Back to RTBC!

🇩🇪Germany Fabianx

RTBC!

Thanks, Nat! The new API looks splendid and works fantastically well locally.

🇩🇪Germany Fabianx

Tested locally!

Works really well.

For those that want to do that on a M1/M2 from the google-drupal repo, here are the steps:

cd .ddev
rm docker-compose.chromedriver.yml
ddev get ddev/ddev-selenium-standalone-chrome

The rest works normally.

Reviewed:

There had been 4 things I flagged:

- a) One nit / question
- b) A suggestion to add a proper API, so that object properties do not need to be used as API
- c) I read now that JS wants to be avoided to load on the page directly, so it's probably answered, but would still be good to make this clear. (as this design decision is only in the chromium bug report)
- d) It's unclear how spans are working in relation to scopes; might be good to add some comments to explain how that works

Overall looks really really good and once these questions are answered, we can go back to RTBC!

🇩🇪Germany Fabianx

Thanks for the answer!

New version does not need the patch anymore.

🇩🇪Germany Fabianx

Should be fixed now, but for LAT / LON will need 'additional_fields' => TRUE.

WKT is best.

🇩🇪Germany Fabianx

This looks great to me now.

Assigning to David for review as this is base system core functionality.

🇩🇪Germany Fabianx

Looking at this again with my core committer head on, I think we should revert the string changes.

While it is technically more correct, we should avoid changing strings as much as possible in D7.

The rest still looks good to me.

🇩🇪Germany Fabianx

Should that not then also be done for all the other query parameters like include/exclude?

🇩🇪Germany Fabianx
+++ b/core/lib/Drupal/Core/Asset/JsCollectionOptimizerLazy.php
@@ -125,13 +125,13 @@ public function optimize(array $js_assets) {
-      $ajax_page_state = $this->requestStack->getCurrentRequest()->query->get('ajax_page_state');
+      $ajax_page_state = $this->requestStack->getCurrentRequest()->get('ajax_page_state');

Is that an intentional change?

🇩🇪Germany Fabianx

Overall the patch looks great.

One thing I wondered:

Do we want to make this the default and remove / deprecate the old ways or do we want to keep the old code around and refactor it a little, so that by nice sub-classing we can provide both ways.

Given how critical this is, it might be prudent to allow via configuration to switch back to the old way.

🇩🇪Germany Fabianx

#185: Just ping me once you have something you want to have reviewed :) - I am also silently and eagerly following.

🇩🇪Germany Fabianx
+++ b/core/modules/system/src/Controller/CssAssetController.php
@@ -0,0 +1,282 @@
+      'Cache-control' => 'private, no-store',

We will need to put this headers into a helper function / service or configurable via the container or settings.

Also I really think we should add Edge-Control: bypass-cache if we go to the lengths here or at least give an akamai-8.x module a change to influence those headers ... (if we don't want to add proprietary headers)

--

Overall looks like a great start to me! Needs some more work obviously ...

Production build https://api.contrib.social 0.61.6-2-g546bc20