KhronosGroup / KhronosGroup/glslang

Memory analysis and proposal

Open
#976 3 comments 1 reaction 0 assignees View on GitHub
Memory
Dominant language
C++
Stars
3.6k
Forks
989
Avg merge
1d 2h
Merged PRs (30d)
31

Description

From looking at all the the issues/PRs labeled as [Memory](https://github.com/KhronosGroup/glslang/labels/Memory), and deciding what do actually do.

## Analysis

### Relevant History

- Glslang stopped using its original Win32 DLL model:
* `DllMain()` originally called to dispatch `DetachProcess()` and `DetachThread()`,
* This code stopped being called.

- Addition of `TShader::preprocess()`:
* this only fit the memory model if used as a substitute for `TShader::parse()`
* no design for both: Externalized preprocessing was never part of the original memory design.

### List of Existing Problems

- Final cleanup is not done by the exposed interfaces of `FinalizeProcess()` or `ShFinalize()`
* #916
* #928
* #389

- No scheme to have both `TShader::preprocess()` and `TShader::parse()` in use together.
* #766
* #275

- Multiple independent uses of glslang within a process are cumbersome because, not knowing about each other, they each want to do one-time initialization and finalization.

- The `TPragmaTable` container itself can leak (the contents are properly pool managed)
* #916

- Looking inside `inUseList` after executing destructor, before deallocating memory. Works, but technically not proper.
* #705

- The original `threadPoolAllocator` is sometimes not freed up? Not yet clear which problem triggers this.
* #916

### Independent Improvements

- Move all the OS-dependent stuff to use C++11 features.
* #826: how orthogonal is this to the above?

- Stop worrying about threads altogether in glslang, use an instancing model for the whole thing
* #389 note, needs a different interface?

## Proposed Plan

- [x] Fix the two existing interfaces `glslang::FinalizeProcess()` or `ShFinalize()` do to full clean up. The important thing to preserve is multiple independent uses get to share the one-time cost of the shared super-global symbol table.

- [x] Do the first one with a hidden mutexed reference count, such that multiple calls within a single process are okay.

- [x] Move `TPragmaTable` container to come from the pool, to avoid need for delete. Subset of #916.

- [x] Understand the issue if `SetThreadPoolAllocator()` is sometimes losing the original, and that needs to be preserved. (#916) Fix any underlying problems.

- [x] Fix the preprocess interface problem. Since this does not make an AST and cannot be linked, and is relatively new, there is probably a simple way of having it clean up it's memory either automatically or through an extra interface call. Don't yet have a specific design; but possibly just having two independent pools that both get deleted.

- [x] Work around the need to look inside `inUseList` after executing its destructor.

- [x] Move to using the C++11 tools instead of OS-specific tools. (#826)

- [ ] Consider an instancing model, but since this changes the interface, it is a more difficult thing to deal with.

Contributor guide

Open the contributing guide

Research direction

Review the listed memory issues (#916, #928, #389, #766, #275, #705, and #826) and the completed items in the proposed plan. The remaining work is to decide whether to introduce an instancing model for glslang, define the changed interface, and establish what completion means for independent uses and cleanup.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
compilers
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.