diff --git a/src/clienttranslator/docs/design.md b/src/clienttranslator/docs/design.md index e15fb50ce96..157c5c0de2c 100644 --- a/src/clienttranslator/docs/design.md +++ b/src/clienttranslator/docs/design.md @@ -108,7 +108,8 @@ silently. ## One table per realm, and what that costs `TranslatorService.GetLocalizationTable` names the table by realm — `GeneratedJSONTable_Server` on -the server, `GeneratedJSONTable_Client` on the client — both under `LocalizationService`. They were +the server, `GeneratedJSONTable_Client` on the client — both under `LocalizationService` once the +service has started (see "The table is filled before it is parented" below). They were a single `GeneratedJSONTable` until `c1a7b0a` (2023), whose message says only "fix localization table replication". The failure it removes is visible in the shape it replaced: with one name the server created the table, it replicated, and the client then wrote its own entries into a @@ -227,6 +228,25 @@ entry all along (`LocalizationEntryParserUtils`); `TableLocaleLoader` forwarded `InstanceLocaleLoader` did not. Now both do, on the source-locale pass, which is where it is derived from anyway. Minting backfilling it was always the accident. +### The table is filled before it is parented + +A write to a table under `LocalizationService` costs milliseconds, an unparented one a fraction of +one ([`engine-behavior.md`](engine-behavior.md), "A write to a parented table costs milliseconds"). +Batching makes that one cost per frame, but a service bag whose `Init` spans several frames (a +frame budget, or an `Init` that yields) still flushes once per frame, and a game's whole startup +registration lands in those frames. So a table `TranslatorService` creates stays unparented until +`Start`, and is parented once. If a flush is already queued when `Start` runs, it lands first and +the table is parented at the end of it: a bag that inits and starts in one frame would otherwise +pay for its only flush parented. + +- A second service in the same realm cannot find an unparented table under `LocalizationService`, + so it is also kept in a module-level map by name. The last holder to be destroyed destroys it; + a started service parents it. +- A table that already sits under `LocalizationService` (saved in the place) is used where it is. +- Costs: GUI auto-localization does not see generated entries before `Start` (a loading screen + shown during `Init` stays in its source text), and a bag that never starts never parents its + table. + ### The table is mirrored Deciding any of the above means knowing what the table currently holds, and `GetEntries` copies and @@ -418,7 +438,8 @@ in the output either way. Anything that needs the strict answer should ask a tra - The harnesses `Init()` their service bag but deliberately never `Start()` it. A bag refuses new service types once started, and building a translator or a loader registers services (a loader, and under it a `TemplateProvider`) — so tests build them in the same window production does, - during `Init`. Nothing in this stack defines `Start`, so nothing is skipped. + during `Init`. `TranslatorService.Start` only parents the table, so the harness tables stay + unparented unless a spec starts the bag; `clearGeneratedTables` clears unparented ones too. - Locale-file replication is **not** covered. Under the test runner `RunService:IsRunning()` is false, so the provider resolves everything locally and synchronously; the tombstone path only runs on a live server/client pair. What the specs do cover is the reactive half — a file diff --git a/src/clienttranslator/docs/engine-behavior.md b/src/clienttranslator/docs/engine-behavior.md index 082c11a2a1d..9e239a7d948 100644 --- a/src/clienttranslator/docs/engine-behavior.md +++ b/src/clienttranslator/docs/engine-behavior.md @@ -58,6 +58,31 @@ What is **not** established is whether `SetEntryExample` overwrites source and c all. It does not need to be: no targeted call lands metadata on its own (see above), so the flush never asks an example write to carry it; see [`design.md`](design.md) ("Two write regimes"). +## A write to a parented table costs milliseconds + +Measured with `os.clock` on a Studio client in a game with ~1,200 PlayerGui instances, writing to a +copy of its 93-entry client table: + +| | Parented to LocalizationService | Unparented | +|---|---|---| +| `SetEntryValue` | 6.8 ms | 0.006 ms | +| `SetEntries` (93 entries) | 7.1 ms | 0.32 ms | +| Parenting the filled table | | 10–14 ms | + +A parented write costs about the same whether it touches one entry or the whole table, which is +consistent with the engine re-running auto-localization over the place on each change. That was not +isolated: the GUI count was never varied, so how the cost scales with the place is not established. +What it does establish is that the cost lands on every flush while the table is parented, which is +why the table is filled before it is parented (see [`design.md`](design.md), "The table is filled +before it is parented"). Pinned by `TranslatorService.spec.lua` ("lands a flush queued at Start +before parenting the table"). + +These numbers say nothing about `INVALIDATION_ENTRY_COST`: with a 93-entry table a full rebuild and +a single targeted write cost the same when parented, so they cannot place the crossover below. + +An unparented table's own `GetTranslator` resolves its entries the same as a parented one: every +spec in the package reads through one before its bag starts. + ## Write cost: assumed, not measured The two write regimes rest on a cost model that was reasoned about rather than profiled: diff --git a/src/clienttranslator/src/Shared/Loaders/LocaleLoaderTestUtils.lua b/src/clienttranslator/src/Shared/Loaders/LocaleLoaderTestUtils.lua index b8badfb4270..d53fc941786 100644 --- a/src/clienttranslator/src/Shared/Loaders/LocaleLoaderTestUtils.lua +++ b/src/clienttranslator/src/Shared/Loaders/LocaleLoaderTestUtils.lua @@ -4,7 +4,7 @@ Builds a real world through `setup()`: a real ServiceBag with the real TranslatorService, plus factories that build the loaders against that bag. Loader - writes land in the real (shared, role-named) LocalizationService table, which is + writes land in the real (shared, role-named) generated table, which is cleared before and after every test for isolation, so tests assert on what actually reached the TranslatorService rather than on a stand-in recording writer. `controller:Destroy()` tears the world down. @@ -27,13 +27,13 @@ local TranslatorService = require("TranslatorService") local LocaleLoaderTestUtils = {} --- Removes any generated tables so each test starts from a clean LocalizationService. function LocaleLoaderTestUtils.clearGeneratedTables() for _, child in LocalizationService:GetChildren() do if child:IsA("LocalizationTable") and string.match(child.Name, "^GeneratedJSONTable_") then child:Destroy() end end + TranslatorService.DestroyDetachedTablesForTesting() end --[[ @@ -63,7 +63,6 @@ function LocaleLoaderTestUtils.setup(options) -- Init but deliberately not Start. A loader is a service that pulls in services of its -- own, and a bag rejects service types added after it has started -- so the tests build -- their loaders in the same window production builds them in (during Init, from the - -- JSONTranslator consuming them). Nothing here defines Start, so nothing is skipped. serviceBag:Init() -- Adds one ".json" StringValue to a folder, as Rojo would. diff --git a/src/clienttranslator/src/Shared/TranslatorService.lua b/src/clienttranslator/src/Shared/TranslatorService.lua index 6ac6b2fed1d..f19804b49fc 100644 --- a/src/clienttranslator/src/Shared/TranslatorService.lua +++ b/src/clienttranslator/src/Shared/TranslatorService.lua @@ -105,6 +105,13 @@ type PendingWrite = { -- already requires (everything goes through TranslatorService). local entryCachesByTable: { [LocalizationTable]: EntryCache } = setmetatable({}, { __mode = "k" }) :: any +type DetachedTable = { + Table: LocalizationTable, + Holders: number, +} + +local detachedTablesByName: { [string]: DetachedTable } = {} + export type TranslatorService = typeof(setmetatable( {} :: { _maid: Maid.Maid, @@ -125,6 +132,8 @@ export type TranslatorService = typeof(setmetatable( _pendingFlushPromise: Promise.Promise<()>?, _localizationWriteCount: number, _localizationRebuildCount: number, + _isStarted: boolean, + _detachedTable: DetachedTable?, }, {} :: typeof({ __index = TranslatorService }) )) @@ -143,6 +152,7 @@ function TranslatorService.Init(self: TranslatorService, serviceBag: ServiceBag. self._flushScheduled = false self._localizationWriteCount = 0 self._localizationRebuildCount = 0 + self._isStarted = false self._forcedLocaleId = self._maid:Add(ValueObject.new(nil)) @@ -150,6 +160,25 @@ function TranslatorService.Init(self: TranslatorService, serviceBag: ServiceBag. self._translator:Mount(self:_observeTranslatorImpl()) end +--[=[ + Parents the localization table if it was created before the service started: writes to a + table under LocalizationService cost milliseconds each, and a service bag whose Init spans + several frames flushes once per frame. A flush already queued lands first, still unparented. + See docs/design.md ("The table is filled before it is parented"). +]=] +function TranslatorService.Start(self: TranslatorService) + self._isStarted = true + if not self._flushScheduled then + self:_parentLocalizationTable() + end +end + +--[=[ + Returns the realm's generated localization table. Before [TranslatorService.Start] a newly + created table is not yet parented to LocalizationService. + + @return LocalizationTable +]=] function TranslatorService.GetLocalizationTable(self: TranslatorService): LocalizationTable if self._localizationTable then return self._localizationTable @@ -159,15 +188,66 @@ function TranslatorService.GetLocalizationTable(self: TranslatorService): Locali local localizationTable = LocalizationService:FindFirstChild(localizationTableName) if not localizationTable then - localizationTable = Instance.new("LocalizationTable") - localizationTable.Name = localizationTableName - localizationTable.Parent = LocalizationService + local detached = detachedTablesByName[localizationTableName] + if not detached then + local newTable = Instance.new("LocalizationTable") + newTable.Name = localizationTableName + detached = { Table = newTable, Holders = 0 } + detachedTablesByName[localizationTableName] = detached + end + detached.Holders += 1 + self._detachedTable = detached + localizationTable = detached.Table end self._localizationTable = localizationTable + if self._isStarted and not self._flushScheduled then + self:_parentLocalizationTable() + end return localizationTable end +--[=[ + Destroys every generated table still waiting to be parented. For test isolation: a spec that + fails before destroying its service would leave its table, entries and all, for the next + service in the realm to find. +]=] +function TranslatorService.DestroyDetachedTablesForTesting() + for name, detached in detachedTablesByName do + detachedTablesByName[name] = nil + detached.Table:Destroy() + end +end + +function TranslatorService._parentLocalizationTable(self: TranslatorService) + local detached = self._detachedTable + if not detached then + return + end + + self._detachedTable = nil + local name = detached.Table.Name + if detachedTablesByName[name] == detached then + detachedTablesByName[name] = nil + detached.Table.Parent = LocalizationService + end +end + +function TranslatorService._releaseDetachedTable(self: TranslatorService) + local detached = self._detachedTable + if not detached then + return + end + + self._detachedTable = nil + detached.Holders -= 1 + local name = detached.Table.Name + if detached.Holders <= 0 and detachedTablesByName[name] == detached then + detachedTablesByName[name] = nil + detached.Table:Destroy() + end +end + function TranslatorService._getLocalizationTableName(self: TranslatorService): string if self._tieRealmService:GetTieRealm() == TieRealms.SERVER then return "GeneratedJSONTable_Server" @@ -878,6 +958,10 @@ function TranslatorService._flushWrites(self: TranslatorService) self:_applyPendingEntries(pendingEntries) end + if self._isStarted then + self:_parentLocalizationTable() + end + -- After the table is written, so an observer that re-reads on this emission sees the -- landed values. Fires for every key in the batch, including ones _applyPendingEntries -- skipped as already-matching -- their data is in the table either way, and a key that @@ -1289,6 +1373,8 @@ function TranslatorService.Destroy(self: TranslatorService) promise:Resolve() end + self:_releaseDetachedTable() + self._maid:DoCleaning() end diff --git a/src/clienttranslator/src/Shared/TranslatorService.spec.lua b/src/clienttranslator/src/Shared/TranslatorService.spec.lua index b91f54c45e5..7c064199de2 100644 --- a/src/clienttranslator/src/Shared/TranslatorService.spec.lua +++ b/src/clienttranslator/src/Shared/TranslatorService.spec.lua @@ -8,8 +8,10 @@ local require = require(script.Parent.loader).load(script) local LocalizationService = game:GetService("LocalizationService") local Jest = require("Jest") +local ServiceBag = require("ServiceBag") local Table = require("Table") local TieRealms = require("TieRealms") +local TranslatorService = require("TranslatorService") local TranslatorTestUtils = require("TranslatorTestUtils") local describe = Jest.Globals.describe @@ -17,17 +19,126 @@ local expect = Jest.Globals.expect local it = Jest.Globals.it describe("TranslatorService:GetLocalizationTable", function() - it("creates a role-named LocalizationTable parented to LocalizationService", function() + it("creates a role-named LocalizationTable, parented to LocalizationService on Start", function() local controller = TranslatorTestUtils.setup() local localizationTable = controller.translatorService:GetLocalizationTable() expect(localizationTable:IsA("LocalizationTable")).toBe(true) expect(localizationTable.Name).toBe(TranslatorTestUtils.EXPECTED_TABLE_NAME) + expect(localizationTable.Parent).toBe(nil) + + controller.serviceBag:Start() + expect(localizationTable.Parent).toBe(LocalizationService) + controller:Destroy() + end) + + it("keeps entries written before Start", function() + local controller = TranslatorTestUtils.setup() + local service = controller.translatorService + + service:SetEntryValue("k.one", "One", "", "en", "One") + controller.flushEntries() + controller.serviceBag:Start() + + local localizationTable = service:GetLocalizationTable() + expect(localizationTable.Parent).toBe(LocalizationService) + expect(TranslatorTestUtils.getEntryMap(localizationTable)["k.one"].Values["en"]).toBe("One") + controller:Destroy() + end) + + it("parents the table at once when created after Start", function() + local controller = TranslatorTestUtils.setup() + controller.serviceBag:Start() + + expect(controller.translatorService:GetLocalizationTable().Parent).toBe(LocalizationService) + controller:Destroy() + end) + + it("leaves a table already under LocalizationService parented", function() + local controller = TranslatorTestUtils.setup() + local existing = Instance.new("LocalizationTable") + existing.Name = TranslatorTestUtils.EXPECTED_TABLE_NAME + existing.Parent = LocalizationService + + local localizationTable = controller.translatorService:GetLocalizationTable() + + expect(localizationTable).toBe(existing) expect(localizationTable.Parent).toBe(LocalizationService) controller:Destroy() end) + it("a started service parents an unparented table it shares", function() + local controller = TranslatorTestUtils.setup() + local first = controller.translatorService:GetLocalizationTable() + + local otherBag = controller.track(ServiceBag.new()) + otherBag:GetService(TranslatorService) + otherBag:Init() + otherBag:Start() + + expect(otherBag:GetService(TranslatorService):GetLocalizationTable()).toBe(first) + expect(first.Parent).toBe(LocalizationService) + controller:Destroy() + end) + + it("lands a flush queued at Start before parenting the table", function() + local controller = TranslatorTestUtils.setup() + local service = controller.translatorService + local localizationTable = service:GetLocalizationTable() + local entriesWhenParented + controller.track(localizationTable:GetPropertyChangedSignal("Parent"):Connect(function() + entriesWhenParented = #localizationTable:GetEntries() + end)) + + service:SetEntryValue("k.one", "One", "", "en", "One") + controller.serviceBag:Start() + expect(localizationTable.Parent).toBe(nil) + + controller.awaitEntriesWritten() + expect(localizationTable.Parent).toBe(LocalizationService) + expect(entriesWhenParented).toBe(1) + controller:Destroy() + end) + + it("destroys a table that was never parented when its last service is destroyed", function() + local controller = TranslatorTestUtils.setup() + + local bag = ServiceBag.new() + bag:GetService(TranslatorService) + bag:Init() + local service = bag:GetService(TranslatorService) :: any + service:SetEntryValue("k.one", "One", "", "en", "One") + service:FlushEntriesForTesting() + local first = service:GetLocalizationTable() + bag:Destroy() + + local canReparent = pcall(function() + first.Parent = LocalizationService + end) + expect(canReparent).toBe(false) + local second = controller.translatorService:GetLocalizationTable() + expect(second).never.toBe(first) + expect(#second:GetEntries()).toBe(0) + controller:Destroy() + end) + + it("keeps a shared unparented table for a service that is still alive", function() + local controller = TranslatorTestUtils.setup() + + local bag = ServiceBag.new() + bag:GetService(TranslatorService) + bag:Init() + local otherService = bag:GetService(TranslatorService) :: any + local shared = otherService:GetLocalizationTable() + expect(controller.translatorService:GetLocalizationTable()).toBe(shared) + bag:Destroy() + + controller.serviceBag:Start() + expect(shared.Parent).toBe(LocalizationService) + controller:Destroy() + end) + it("caches the table so repeated calls return the same instance", function() local controller = TranslatorTestUtils.setup() diff --git a/src/clienttranslator/src/Shared/TranslatorTestUtils.lua b/src/clienttranslator/src/Shared/TranslatorTestUtils.lua index db9040d4489..52d905e0a4d 100644 --- a/src/clienttranslator/src/Shared/TranslatorTestUtils.lua +++ b/src/clienttranslator/src/Shared/TranslatorTestUtils.lua @@ -5,8 +5,8 @@ Builds a real world through `setup()`: a real ServiceBag with the real TranslatorService, translator factories initialized against it, and await helpers for the (async) Roblox translator. Writes land in the real (shared, - role-named) LocalizationService table, which is cleared before and after every - test for isolation. `controller:Destroy()` tears the world down. + role-named) generated table, left unparented unless a test starts the bag, which is + cleared before and after every test for isolation. `controller:Destroy()` tears the world down. @class TranslatorTestUtils ]] @@ -39,13 +39,13 @@ function TranslatorTestUtils.getEntryMap(localizationTable) return map end --- Removes any generated tables so each test starts from a clean LocalizationService. function TranslatorTestUtils.clearGeneratedTables() for _, child in LocalizationService:GetChildren() do if child:IsA("LocalizationTable") and string.match(child.Name, "^GeneratedJSONTable_") then child:Destroy() end end + TranslatorService.DestroyDetachedTablesForTesting() end --[[ @@ -77,7 +77,6 @@ function TranslatorTestUtils.setup(options) -- Init but deliberately not Start: the translator factories below initialize their -- translators against this bag, and a translator registers its loader (which -- registers a TemplateProvider) as it initializes. A bag rejects service types added - -- after it has started. Nothing in this stack defines Start, so nothing is skipped. serviceBag:Init() return serviceBag:GetService(TranslatorService), serviceBag end