Skip to content

Off by one AST cache cleanup #92

@germainpigeon

Description

@germainpigeon

There is a behaviour that is unconsistent with the comment here:

// Clear the AST cache when it hits 1024 entries
if (++$this->cachedCount > 1024) {
$this->cache = [];
$this->cachedCount = 0;
}
$this->cache[$expression] = $this->parser->parse($expression);

  • After the first call cachedCount will be 1 and the cache will have 1 entry.
  • After the 1024th call cachedCount will be 1024 and the cache will have 1024 entries.
    👉Note that while the cache has hit 1024 entries it is not cleared yet, so to me the comment is already a little misleading.
  • After the 1025th call cachedCount will be 0 and the cache will have 1 entry.
    👉One could say that the state of the object is now invalid ?
  • After the 2048th call cachedCount will be 1023 and the cache will have 1024 entry.
    👉Note that while the cache has hit 1024 entries it is not cleared yet, once again.
  • After the 2049th call cachedCount will be 1024 and the cache will have 1025 entry.
    ❌This is more than 1024 and to me is not what was expected by the developper who wrote the comment.
  • After the 2050th call cachedCount will be 0 and the cache will have 1 entry.
    👉Again, one could say that the state of the object is now invalid
  • (Then it will repeat)

Please have a look to the code illustrating the current behaviour on 3v4l:
https://3v4l.org/MsW7e

Side proposition : rewrite the comment

-             // Clear the AST cache when it hits 1024 entries
+             // Clear the AST cache if it already holds 1024 entries 

Proposition 1 : fastest fix

-                 $this->cachedCount = 0;
+                 $this->cachedCount = 1;

Proposition 2 : cleaner code (as in "easier to review")

-             if (++$this->cachedCount > 1024) {
+             if ($this->cachedCount > 1024) {
                  $this->cache = [];
                  $this->cachedCount = 0;
              }
              $this->cache[$expression] = $this->parser->parse($expression);
+             $this->cachedCount++;

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions