Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CVar to disable pow3r parallel processing #124

Closed
wants to merge 10 commits into from
Closed

Add CVar to disable pow3r parallel processing #124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
35d48cf
fix: The thrown item is now triggered once
Spatison Sep 1, 2024
fd2545f
fix: The powered airlock are now glowing
Spatison Sep 1, 2024
108374a
tweak: An ordinary player can now use adminwho
Spatison Sep 1, 2024
1a39b5c
fix: The powered airlock are now glowing
Spatison Sep 2, 2024
ab14630
tweak: neglecting time when using scrap
Spatison Sep 2, 2024
3ea1539
add: WD Airlock texture
Spatison Sep 2, 2024
e65c240
fix
Spatison Sep 2, 2024
36341b5
fix
Spatison Sep 2, 2024
48e362b
Add CVar to disable pow3r parallel processing (#28488)
Partmedia Jun 4, 2024
5bbd6b4
Merge branch 'WWhiteDreamProject:fix' into fix
Vaaankas Nov 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
13 changes: 12 additions & 1 deletion Content.Server/Power/EntitySystems/PowerNetSystem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@
using Content.Server.Power.Components;
using Content.Server.Power.NodeGroups;
using Content.Server.Power.Pow3r;
using Content.Shared.CCVar;
using Content.Shared.Power;
using JetBrains.Annotations;
using Robust.Server.GameObjects;
using Robust.Shared.Configuration;
using Robust.Shared.Threading;

namespace Content.Server.Power.EntitySystems
Expand All @@ -18,19 +20,21 @@ public sealed class PowerNetSystem : EntitySystem
{
[Dependency] private readonly AppearanceSystem _appearance = default!;
[Dependency] private readonly PowerNetConnectorSystem _powerNetConnector = default!;
[Dependency] private readonly IConfigurationManager _cfg = default!;
[Dependency] private readonly IParallelManager _parMan = default!;

private readonly PowerState _powerState = new();
private readonly HashSet<PowerNet> _powerNetReconnectQueue = new();
private readonly HashSet<ApcNet> _apcNetReconnectQueue = new();

private readonly BatteryRampPegSolver _solver = new();
private BatteryRampPegSolver _solver = new();

public override void Initialize()
{
base.Initialize();

UpdatesAfter.Add(typeof(NodeGroupSystem));
_solver = new(_cfg.GetCVar(CCVars.DebugPow3rDisableParallel));

SubscribeLocalEvent<ApcPowerReceiverComponent, ComponentInit>(ApcPowerReceiverInit);
SubscribeLocalEvent<ApcPowerReceiverComponent, ComponentShutdown>(ApcPowerReceiverShutdown);
Expand All @@ -52,6 +56,13 @@ public override void Initialize()
SubscribeLocalEvent<PowerSupplierComponent, ComponentShutdown>(PowerSupplierShutdown);
SubscribeLocalEvent<PowerSupplierComponent, EntityPausedEvent>(PowerSupplierPaused);
SubscribeLocalEvent<PowerSupplierComponent, EntityUnpausedEvent>(PowerSupplierUnpaused);

Subs.CVar(_cfg, CCVars.DebugPow3rDisableParallel, DebugPow3rDisableParallelChanged);
}

private void DebugPow3rDisableParallelChanged(bool val)
{
_solver = new(val);
Comment on lines +63 to +65
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Optimize solver recreation and add error handling

The configuration change handler could benefit from these improvements:

  1. Add validation to prevent unnecessary solver recreation
  2. Include error handling for solver creation
  3. Consider adding logging for debugging purposes
         private void DebugPow3rDisableParallelChanged(bool val)
         {
-            _solver = new(val);
+            try 
+            {
+                if (_solver.DisableParallel != val)
+                {
+                    Logger.Debug($"Updating power solver parallel processing: {!val}");
+                    _solver = new(val);
+                }
+            }
+            catch (Exception ex)
+            {
+                Logger.Error($"Failed to update power solver configuration: {ex}");
+            }
         }

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +63 to +65
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add thread safety and error handling to configuration change handler

The current implementation might be unsafe if the solver is being used when reconfiguration occurs. Consider adding thread synchronization and error handling.

-        private void DebugPow3rDisableParallelChanged(bool val)
-        {
-            _solver = new(val);
-        }
+        private readonly object _solverLock = new object();
+        
+        private void DebugPow3rDisableParallelChanged(bool val)
+        {
+            try
+            {
+                lock (_solverLock)
+                {
+                    _solver = new(val);
+                }
+            }
+            catch (Exception ex)
+            {
+                Logger.Error($"Failed to reconfigure solver: {ex}");
+                // Consider reverting the configuration value
+                _cfg.SetCVar(CCVars.DebugPow3rDisableParallel, !val);
+            }
+        }

Also, ensure that the Update method uses the same lock:

         public override void Update(float frameTime)
         {
             base.Update(frameTime);
 
             ReconnectNetworks();
 
             // Synchronize batteries
             RaiseLocalEvent(new NetworkBatteryPreSync());
 
             // Run power solver.
-            _solver.Tick(frameTime, _powerState, _parMan);
+            lock (_solverLock)
+            {
+                _solver.Tick(frameTime, _powerState, _parMan);
+            }

Committable suggestion skipped: line range outside the PR's diff.

}

private void ApcPowerReceiverInit(EntityUid uid, ApcPowerReceiverComponent component, ComponentInit args)
Expand Down
9 changes: 7 additions & 2 deletions Content.Server/Power/Pow3r/BatteryRampPegSolver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@ namespace Content.Server.Power.Pow3r
public sealed class BatteryRampPegSolver : IPowerSolver
{
private UpdateNetworkJob _networkJob;
private bool _disableParallel;

public BatteryRampPegSolver()
public BatteryRampPegSolver(bool disableParallel = false)
{
_disableParallel = disableParallel;
_networkJob = new()
{
Solver = this,
Expand Down Expand Up @@ -54,7 +56,10 @@ public void Tick(float frameTime, PowerState state, IParallelManager parallel)
// suppliers + discharger) Then decide based on total layer size whether its worth parallelizing that
// layer?
_networkJob.Networks = group;
parallel.ProcessNow(_networkJob, group.Count);
if (_disableParallel)
parallel.ProcessSerialNow(_networkJob, group.Count);
else
parallel.ProcessNow(_networkJob, group.Count);
}

ClearBatteries(state);
Expand Down
6 changes: 6 additions & 0 deletions Content.Shared/CCVar/CCVars.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2619,5 +2619,11 @@ public static readonly CVarDef<float>
CVarDef.Create("ghost.allow_same_character", false, CVar.SERVERONLY);

#endregion

/// <summary>
/// Set to true to disable parallel processing in the pow3r solver.
/// </summary>
public static readonly CVarDef<bool> DebugPow3rDisableParallel =
CVarDef.Create("debug.pow3r_disable_parallel", true, CVar.SERVERONLY);
}
}
54 changes: 27 additions & 27 deletions Resources/Prototypes/Entities/Structures/Doors/Airlocks/airlocks.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
suffix: Freezer
components:
- type: Sprite
sprite: Structures/Doors/Airlocks/Standard/freezer.rsi
sprite: _White/Structures/Doors/Airlocks/Standard/freezer.rsi # WD EDIT
- type: Wires
layoutId: AirlockService

Expand All @@ -14,7 +14,7 @@
suffix: Engineering
components:
- type: Sprite
sprite: Structures/Doors/Airlocks/Standard/engineering.rsi
sprite: _White/Structures/Doors/Airlocks/Standard/engineering.rsi # WD EDIT
- type: PaintableAirlock
department: Engineering
- type: Wires
Expand All @@ -26,15 +26,15 @@
suffix: Atmospherics
components:
- type: Sprite
sprite: Structures/Doors/Airlocks/Standard/atmospherics.rsi
sprite: _White/Structures/Doors/Airlocks/Standard/atmospherics.rsi # WD EDIT

- type: entity
parent: Airlock
id: AirlockCargo
suffix: Logistics # DeltaV - Logistics Department replacing Cargo
components:
- type: Sprite
sprite: Structures/Doors/Airlocks/Standard/cargo.rsi
sprite: _White/Structures/Doors/Airlocks/Standard/cargo.rsi # WD EDIT
- type: PaintableAirlock
department: Logistics
- type: Wires
Expand All @@ -46,7 +46,7 @@
suffix: Medical
components:
- type: Sprite
sprite: Structures/Doors/Airlocks/Standard/medical.rsi
sprite: _White/Structures/Doors/Airlocks/Standard/medical.rsi # WD EDIT
- type: PaintableAirlock
department: Medical
- type: Wires
Expand All @@ -58,7 +58,7 @@
suffix: Virology
components:
- type: Sprite
sprite: Structures/Doors/Airlocks/Standard/virology.rsi
sprite: _White/Structures/Doors/Airlocks/Standard/virology.rsi # WD EDIT

- type: entity
parent: AirlockMedical
Expand All @@ -71,7 +71,7 @@
suffix: Epistemics # DeltaV - Epistemics Department replacing Science
components:
- type: Sprite
sprite: Structures/Doors/Airlocks/Standard/science.rsi
sprite: _White/Structures/Doors/Airlocks/Standard/science.rsi # WD EDIT
- type: PaintableAirlock
department: Epistemics
- type: Wires
Expand All @@ -83,7 +83,7 @@
suffix: Command
components:
- type: Sprite
sprite: Structures/Doors/Airlocks/Standard/command.rsi
sprite: _White/Structures/Doors/Airlocks/Standard/command.rsi # WD EDIT
- type: WiresPanelSecurity
securityLevel: medSecurity
- type: PaintableAirlock
Expand All @@ -97,7 +97,7 @@
suffix: Security
components:
- type: Sprite
sprite: Structures/Doors/Airlocks/Standard/security.rsi
sprite: _White/Structures/Doors/Airlocks/Standard/security.rsi # WD EDIT
- type: PaintableAirlock
department: Security
- type: Wires
Expand All @@ -109,23 +109,23 @@
name: maintenance access
components:
- type: Sprite
sprite: Structures/Doors/Airlocks/Standard/maint.rsi
sprite: _White/Structures/Doors/Airlocks/Standard/maint.rsi # WD EDIT

- type: entity
parent: AirlockSecurity # if you get syndie door somehow it counts as sec
id: AirlockSyndicate
suffix: Syndicate
components:
- type: Sprite
sprite: Structures/Doors/Airlocks/Standard/syndicate.rsi
sprite: _White/Structures/Doors/Airlocks/Standard/syndicate.rsi # WD EDIT

- type: entity
parent: AirlockCargo
id: AirlockMining
suffix: Mining(Salvage)
components:
- type: Sprite
sprite: Structures/Doors/Airlocks/Standard/mining.rsi
sprite: _White/Structures/Doors/Airlocks/Standard/mining.rsi # WD EDIT
- type: Wires
layoutId: AirlockCargo

Expand All @@ -135,23 +135,23 @@
suffix: Central Command
components:
- type: Sprite
sprite: Structures/Doors/Airlocks/Standard/centcomm.rsi
sprite: _White/Structures/Doors/Airlocks/Standard/centcomm.rsi # WD EDIT

- type: entity
parent: Airlock
id: AirlockHatch
name: airtight hatch
components:
- type: Sprite
sprite: Structures/Doors/Airlocks/Standard/hatch.rsi
sprite: _White/Structures/Doors/Airlocks/Standard/hatch.rsi # WD EDIT

- type: entity
parent: Airlock
id: AirlockHatchMaintenance
name: maintenance hatch
components:
- type: Sprite
sprite: Structures/Doors/Airlocks/Standard/hatch_maint.rsi
sprite: _White/Structures/Doors/Airlocks/Standard/hatch_maint.rsi # WD EDIT

# Glass
- type: entity
Expand All @@ -160,7 +160,7 @@
suffix: Engineering
components:
- type: Sprite
sprite: Structures/Doors/Airlocks/Glass/engineering.rsi
sprite: _White/Structures/Doors/Airlocks/Glass/engineering.rsi # WD EDIT
- type: PaintableAirlock
department: Engineering
- type: Wires
Expand All @@ -172,23 +172,23 @@
suffix: Maintenance
components:
- type: Sprite
sprite: Structures/Doors/Airlocks/Glass/maint.rsi
sprite: _White/Structures/Doors/Airlocks/Glass/maint.rsi # WD EDIT

- type: entity
parent: AirlockEngineeringGlass
id: AirlockAtmosphericsGlass
suffix: Atmospherics
components:
- type: Sprite
sprite: Structures/Doors/Airlocks/Glass/atmospherics.rsi
sprite: _White/Structures/Doors/Airlocks/Glass/atmospherics.rsi # WD EDIT

- type: entity
parent: AirlockGlass
id: AirlockCargoGlass
suffix: Logistics # DeltaV - Logistics Department replacing Cargo
components:
- type: Sprite
sprite: Structures/Doors/Airlocks/Glass/cargo.rsi
sprite: _White/Structures/Doors/Airlocks/Glass/cargo.rsi # WD EDIT
- type: PaintableAirlock
department: Logistics
- type: Wires
Expand All @@ -200,7 +200,7 @@
suffix: Medical
components:
- type: Sprite
sprite: Structures/Doors/Airlocks/Glass/medical.rsi
sprite: _White/Structures/Doors/Airlocks/Glass/medical.rsi # WD EDIT
- type: PaintableAirlock
department: Medical
- type: Wires
Expand All @@ -217,15 +217,15 @@
suffix: Virology
components:
- type: Sprite
sprite: Structures/Doors/Airlocks/Glass/virology.rsi
sprite: _White/Structures/Doors/Airlocks/Glass/virology.rsi # WD EDIT

- type: entity
parent: AirlockGlass
id: AirlockScienceGlass
suffix: Epistemics # DeltaV - Epistemics Department replacing Science
components:
- type: Sprite
sprite: Structures/Doors/Airlocks/Glass/science.rsi
sprite: _White/Structures/Doors/Airlocks/Glass/science.rsi # WD EDIT
- type: PaintableAirlock
department: Epistemics
- type: Wires
Expand All @@ -237,7 +237,7 @@
suffix: Command
components:
- type: Sprite
sprite: Structures/Doors/Airlocks/Glass/command.rsi
sprite: _White/Structures/Doors/Airlocks/Glass/command.rsi # WD EDIT
- type: PaintableAirlock
department: Command
- type: WiresPanelSecurity
Expand All @@ -251,7 +251,7 @@
suffix: Security
components:
- type: Sprite
sprite: Structures/Doors/Airlocks/Glass/security.rsi
sprite: _White/Structures/Doors/Airlocks/Glass/security.rsi # WD EDIT
- type: PaintableAirlock
department: Security
- type: Wires
Expand All @@ -263,20 +263,20 @@
suffix: Syndicate
components:
- type: Sprite
sprite: Structures/Doors/Airlocks/Glass/syndicate.rsi
sprite: _White/Structures/Doors/Airlocks/Glass/syndicate.rsi # WD EDIT

- type: entity
parent: AirlockCargoGlass
id: AirlockMiningGlass
suffix: Mining(Salvage)
components:
- type: Sprite
sprite: Structures/Doors/Airlocks/Glass/mining.rsi
sprite: _White/Structures/Doors/Airlocks/Glass/mining.rsi # WD EDIT

- type: entity
parent: AirlockCommandGlass # see standard
id: AirlockCentralCommandGlass
suffix: Central Command
components:
- type: Sprite
sprite: Structures/Doors/Airlocks/Glass/centcomm.rsi
sprite: _White/Structures/Doors/Airlocks/Glass/centcomm.rsi # WD EDIT
Loading
Loading