Browse Source

Fix issues

pull/1330/head
Sebastian Stehle 2 weeks ago
parent
commit
bd695c1f46
  1. 4
      backend/src/Squidex.Domain.Apps.Entities/Jobs/JobWorker.cs
  2. 11
      backend/src/Squidex.Infrastructure/Translations/ResourcesLocalizer.cs
  3. 28
      backend/tests/Squidex.Infrastructure.Tests/Translations/TTests.cs
  4. 40
      resolved.md
  5. 48
      todo.md

4
backend/src/Squidex.Domain.Apps.Entities/Jobs/JobWorker.cs

@ -68,7 +68,7 @@ public sealed class JobWorker :
return GetJobProcessorAsync(message.OwnerId); return GetJobProcessorAsync(message.OwnerId);
} }
private Task<JobProcessor> GetJobProcessorAsync(DomainId appId) private async Task<JobProcessor> GetJobProcessorAsync(DomainId appId)
{ {
Task<JobProcessor> processor; Task<JobProcessor> processor;
lock (processors) lock (processors)
@ -82,7 +82,7 @@ public sealed class JobWorker :
return loaded; return loaded;
}); });
} }
try try
{ {
return await processor; return await processor;

11
backend/src/Squidex.Infrastructure/Translations/ResourcesLocalizer.cs

@ -5,7 +5,9 @@
// All rights reserved. Licensed under the MIT license. // All rights reserved. Licensed under the MIT license.
// ========================================================================== // ==========================================================================
using System.Collections.Concurrent;
using System.Globalization; using System.Globalization;
using System.Reflection;
using System.Resources; using System.Resources;
using System.Text; using System.Text;
@ -13,6 +15,13 @@ namespace Squidex.Infrastructure.Translations;
public sealed class ResourcesLocalizer(ResourceManager resourceManager) : ILocalizer public sealed class ResourcesLocalizer(ResourceManager resourceManager) : ILocalizer
{ {
// Resolving a property by name is the expensive part of the formatting. The arguments are
// compiler generated types and the variable names come from the resources, so the number of
// combinations is defined by the code and the cache cannot grow beyond that. Misses are cached
// as null, otherwise an unknown variable would pay for the lookup every time.
private static readonly ConcurrentDictionary<(Type Type, string Name), PropertyInfo?> Properties =
new ConcurrentDictionary<(Type Type, string Name), PropertyInfo?>();
public (string Result, bool Found) Get(CultureInfo culture, string key, string fallback, object? args = null) public (string Result, bool Found) Get(CultureInfo culture, string key, string fallback, object? args = null)
{ {
Guard.NotNull(culture); Guard.NotNull(culture);
@ -74,7 +83,7 @@ public sealed class ResourcesLocalizer(ResourceManager resourceManager) : ILocal
var variableName = variable.ToString(); var variableName = variable.ToString();
var variableValue = variableName; var variableValue = variableName;
var property = argsType.GetProperty(variableName); var property = Properties.GetOrAdd((argsType, variableName), static key => key.Type.GetProperty(key.Name));
if (property != null) if (property != null)
{ {

28
backend/tests/Squidex.Infrastructure.Tests/Translations/TTests.cs

@ -57,4 +57,32 @@ public class TTests
Assert.Equal(("Var: Upper.", true), actual); Assert.Equal(("Var: Upper.", true), actual);
} }
[Fact]
public void Should_return_variable_name_when_property_does_not_exist()
{
var actual = sut.Get(CultureInfo.CurrentUICulture, "withVar", "fallback", new { other = 5 });
Assert.Equal(("Var: var.", true), actual);
}
[Fact]
public void Should_return_same_text_when_called_again()
{
var actual1 = sut.Get(CultureInfo.CurrentUICulture, "withVar", "fallback", new { var = 5 });
var actual2 = sut.Get(CultureInfo.CurrentUICulture, "withVar", "fallback", new { var = 8 });
Assert.Equal(("Var: 5.", true), actual1);
Assert.Equal(("Var: 8.", true), actual2);
}
[Fact]
public void Should_not_reuse_property_of_other_type()
{
var actual1 = sut.Get(CultureInfo.CurrentUICulture, "withVar", "fallback", new { var = 5 });
var actual2 = sut.Get(CultureInfo.CurrentUICulture, "withVar", "fallback", new { var = "text" });
Assert.Equal(("Var: 5.", true), actual1);
Assert.Equal(("Var: text.", true), actual2);
}
} }

40
resolved.md

@ -1194,3 +1194,43 @@ one. The same defect as item 15.
**Now:** the entry is removed when the task faults, comparing by reference so a newer **Now:** the entry is removed when the task faults, comparing by reference so a newer
successful entry added by another caller is not discarded. The removal takes the same lock successful entry added by another caller is not discarded. The removal takes the same lock
that guards the dictionary. that guards the dictionary.
---
### 34. Message formatting looked up properties by reflection on every call — **FIXED**
`backend/src/Squidex.Infrastructure/Translations/ResourcesLocalizer.cs`
**Was:** `ResourcesLocalizer.Get` resolved every `{variable}` placeholder with
`argsType.GetProperty(variableName)` and cached nothing, so each call paid a name search over
the type's members. Mostly harmless on error paths, but `ResolveReferences.CreateFallback`
calls `T.Get("contents.listReferences", new { count = … })` inside the per-content,
per-partition loop of the enrichment pipeline.
**Now:** a static `ConcurrentDictionary<(Type, string), PropertyInfo?>` memoizes the lookup.
**Two details that matter more than they look:**
- **Misses are cached as `null`.** An unknown variable name is a legitimate case — the code
falls back to printing the name — and without caching the negative result those would pay
the reflection cost on every single call, which is the worst case rather than the best.
- **The key is (Type, name), not name.** Different anonymous types share property names, so a
name-only key would hand back another type's `PropertyInfo` and `GetValue` would throw into
the existing `catch`, silently degrading the message to the raw variable name.
No eviction: the arg types are compiler-generated and the variable names come from the
resource files, so the number of combinations is fixed by the code.
**Verified:** three new tests in `TTests` — an unknown property, repeated calls with different
values, and two different arg types using the same variable name. The last one is the guard
against the name-only key: mutating the implementation to key by name alone fails it, along
with the two existing case-conversion tests, and leaves the rest passing.
`Squidex.Infrastructure.Tests` green (1036).
---
### Note: `JobWorker` build fix
While running the suites, `JobWorker.GetJobProcessorAsync` did not compile — the helper from
item 35 had been inlined into it but the method was still declared non-async while using
`await`. Added the `async` modifier and removed a trailing whitespace. No behavioural change;
the `lock` block closes before the `await`, so nothing is held across it.

48
todo.md

@ -9,9 +9,9 @@ overhead / allocation churn), **S4** low (worth fixing while nearby).
Item numbers are stable and never reused. Completed items move to Item numbers are stable and never reused. Completed items move to
[resolved.md](resolved.md) keeping their number, so gaps in the sequence here are [resolved.md](resolved.md) keeping their number, so gaps in the sequence here are
expected — items **4**–**33** and **35** are closed and live there. expected — items **4**–**35** are closed and live there.
**Status: 4 open of 35 — items 1, 2, 3 (one root cause) and 34. The other 31 are in [resolved.md](resolved.md).** **Status: 3 open of 35 — items 1, 2 and 3, which are one root cause. The other 32 are in [resolved.md](resolved.md).**
--- ---
@ -66,45 +66,15 @@ already isolated in `ContentScriptVars`.
--- ---
## S3 — Moderate
### 34. Message formatting looks up properties by reflection on every call
`backend/src/Squidex.Infrastructure/Translations/ResourcesLocalizer.cs:77`
`ResourcesLocalizer.Get` substitutes `{variable}` placeholders by reflecting over the
anonymous args object:
```csharp
var property = argsType.GetProperty(variableName);
```
No `PropertyInfo` is cached, so every call re-resolves it. `Type.GetProperty(string)` is one
of the slower reflection calls — it does a name lookup over the type's members.
Mostly this sits on error paths, where it does not matter. The one that is not is
`ResolveReferences.CreateFallback` (`ResolveReferences.cs:125`):
```csharp
var text = T.Get("contents.listReferences", new { count = referencedContents.Count });
```
That runs inside the per-content, per-partition loop of the enrichment pipeline, for every
reference field that resolves to more than one item. The same method also allocates a fresh
`JsonObject` and loops over every app language on each call.
**Fix:** cache the `PropertyInfo` per (type, name); a small static dictionary is enough since
the arg types are compiler-generated and few.
---
## Suggested order of attack ## Suggested order of attack
1. **Item 34** — small and self-contained: cache the `PropertyInfo` per (type, name). Only **engine pooling (items 1–3)** is left. It is the largest single cost on the list and
2. **Items 1–3, engine pooling** — the largest single cost, and the most invasive change on also the most invasive change: it touches the security boundary of user-authored scripts,
the list. It touches the security boundary of user-authored scripts, since a pooled because a pooled engine must not carry state from one script into the next.
engine must not carry state from one script into the next. **Profile before writing it**:
the estimate that engine construction dominates a scripted content list is read off the **Profile before writing it.** The estimate that engine construction dominates a scripted
loops, not taken from a trace. content list is read off the loops, not taken from a trace, and this is the one item where
the fix is expensive enough that being wrong about the size of the win would matter.
--- ---

Loading…
Cancel
Save