fix(clienttranslator): Fill the generated table before parenting it

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

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:

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 "<locale>.json" StringValue to a folder, as Rojo would.

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,13 +152,33 @@ 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))
 
 	self._translator = self._maid:Add(ValueObject.new(nil))
 	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
 

src/clienttranslator/src/Shared/TranslatorService.spec.lua

@@ -8,26 +8,137 @@ 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
 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()