Issue 1257 fix mem calc vars and update compare doc#1258
Open
stokpop wants to merge 6 commits intocloudfoundry:mainfrom
Open
Issue 1257 fix mem calc vars and update compare doc#1258stokpop wants to merge 6 commits intocloudfoundry:mainfrom
stokpop wants to merge 6 commits intocloudfoundry:mainfrom
Conversation
…undry#1257) - Add memory_calculator_issues_test.go with 4 failing tests documenting bugs tracked in cloudfoundry#1257: * MEMORY_CALCULATOR_STACK_THREADS ignored (LoadConfig() not called) * MEMORY_CALCULATOR_HEADROOM ignored (LoadConfig() not called) * JBP_CONFIG_OPEN_JDK_JRE stack_threads not parsed * JBP_CONFIG_OPEN_JDK_JRE class_count not parsed - Update RUBY_VS_GO_BUILDPACK_COMPARISON.md: memory calculator is no longer 'Identical' — v4 includes user-pinned -Xmx in total memory check (v3 did not), which can cause startup failures in small containers after migration from the Ruby buildpack
…fig() in Supply/Finalize - Add openJDKJREConfig / memoryCalculatorConfig structs for YAML parsing - Rewrite LoadConfig() to parse JBP_CONFIG_OPEN_JDK_JRE using common.YamlHandler (standard CF config pattern); MEMORY_CALCULATOR_* env vars remain supported as a more specific override - Guard with configLoaded flag to prevent double-loading when both Supply() and Finalize() are called on the same instance - Call LoadConfig() at start of Supply() so class_count override takes effect before countClasses() runs; skip countClasses() if already set - Call LoadConfig() in Finalize() so stackThreads/headroom/classCount are applied when building the calculator command (separate process) Fixes: MEMORY_CALCULATOR_STACK_THREADS/HEADROOM silently ignored Fixes: JBP_CONFIG_OPEN_JDK_JRE stack_threads/class_count silently ignored
…DK_JRE Avoids spurious WARNING when jre: or other valid top-level keys are present in JBP_CONFIG_OPEN_JDK_JRE alongside memory_calculator:. Previously ValidateFields was run on the full config struct, which caused warnings for unknown fields like 'jre' that are handled elsewhere in the buildpack. Now only the memory_calculator sub-section is validated, so typos in memory_calculator fields (e.g. stack_thread instead of stack_threads) still produce a WARNING while legitimate top-level keys are silently ignored. Added two tests: - no WARNING when jre: is present alongside memory_calculator: - WARNING when memory_calculator: contains an unknown/typo'd field
Log message now shows whether each value was user-supplied or not: - Loaded Classes: 23826 (auto-detected) vs 23826 when user-set - Threads: 250 (default) vs 50 when user-set - Headroom: 0% (default) vs 5% when user-set Adds classCountUserSet, stackThreadsUserSet, headroomUserSet flags to MemoryCalculator set during LoadConfig() when a value comes from JBP_CONFIG_OPEN_JDK_JRE or MEMORY_CALCULATOR_* env vars.
- Fix table: show that v3 squeezes non-heap to fit, v4 calculates non-heap independently and correctly includes fixed -Xmx in check - Remove incorrect '(or via JBP_CONFIG_*)' — -Xmx can only be set in JAVA_OPTS, not via JBP_CONFIG_* - Remove stale 'known bug' note about JBP_CONFIG_OPEN_JDK_JRE parsing (now fixed) - Add explanation that v4 behaviour is correct: v3 silently undersized non-heap (thread stacks, metaspace, code cache) when -Xmx was set - Reorder migration options: lower stack_threads moved to option 2 - Add virtual threads note: carrier threads only consume native stack memory; virtual thread continuations live on the heap — set stack_threads to carrier thread count (~CPU cores) for Java 21+ apps - Use realistic memory estimates (~900-1000M for -Xmx512M app)
- Clarify the v3 vs v4 difference only applies when explicit -Xmx is set - Note that without explicit -Xmx the startup failure does not occur - Note v3 claimed less memory by squeezing non-heap alongside fixed -Xmx - Reorder migration options: lower stack_threads first, with caveat to only do so when app actually uses fewer than 250 threads - Clarify that removing -Xmx still likely requires increasing manifest memory - Fix memory figure to at least 1300M based on actual observed error
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1257 — Memory calculator config not applied at startup
What changed
Bug fixes
LoadConfig()is now called in bothSupply()andFinalize(), so memory calculator settings are applied at staging time (needed for class counting) and at runtime command generation.JBP_CONFIG_OPEN_JDK_JREis now parsed formemory_calculatorsettings (stack_threads,headroom,class_count). Previously these were silently ignored.ValidateFieldsis scoped to thememory_calculatorsub-section only, so legitimate top-level keys likejre:no longer produce a spuriousWARNING.Improved logging
Memory Calculator installed: Loaded Classes: 23826 (auto-detected), Threads: 250 (default), Headroom: 0% (default)Documentation
RUBY_VS_GO_BUILDPACK_COMPARISON.md: updated memory calculator row and added migration section explaining the v3 → v4 behaviour change. The difference only affects apps with an explicit-XmxinJAVA_OPTS(bad practice in containerised environments). Migration options: lowerstack_threads, remove-Xmx, or increase manifest memory.Tests
memory_calculator_issues_test.gocovering the above fixes.