Conversation
|
Fix #884 |
There was a problem hiding this comment.
Pull request overview
This PR updates the JIT decoder’s float32 decoding path to match Go’s standard library behavior, addressing precision-compatibility differences by parsing float32 values using stdlib semantics.
Changes:
- Add a stdlib-based float32 parsing helper (
decodeFloat32Std) usingstrconv.ParseFloat(..., 32). - Switch JIT op implementations for float32 values and float32 map keys to use the stdlib-based parsing path.
- Add test coverage for float32 precision-compatibility cases in both JIT-level and package-level tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| internal/decoder/jitdec/primitives.go | Adds decodeFloat32Std helper for stdlib float32 parsing. |
| internal/decoder/jitdec/assembler_regabi_amd64.go | Updates _OP_f32 and _OP_map_key_f32 to decode float32 via decodeFloat32Std. |
| internal/decoder/jitdec/assembler_test.go | Adds JIT opcode tests for float32 precision-compatibility (value + map key). |
| decoder/decoder_test.go | Adds package-level test to assert float32 decoding matches encoding/json. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.parse_number(float32Type, pin, -1) // PARSE NUMBER | ||
| self.slice_from(_VAR_st_Ep, 0) // SLICE st.Ep, $0 | ||
| self.Emit("MOVQ", _DI, _AX) // MOVQ DI, AX | ||
| self.Emit("MOVQ", _SI, _BX) // MOVQ SI, BX | ||
| self.Emit("MOVQ", _VP, _CX) // MOVQ VP, CX | ||
| self.call_go(_F_decodeFloat32Std) // CALL_GO decodeFloat32Std | ||
| self.Emit("TESTQ", _ET, _ET) // TESTQ ET, ET |
There was a problem hiding this comment.
_asm_OP_f32 currently parses the number twice: once via parse_number/vnumber and again via strconv.ParseFloat in decodeFloat32Std (the st.Dv result from the first parse is unused). If possible, restructure this path to avoid double-parsing (e.g., locate the numeric token end with skip_number/indices, slice, then parse once with stdlib float32 semantics).
| self.parse_number(float32Type, "", p.vi()) // PARSE NUMBER | ||
| self.slice_from(_VAR_st_Ep, 0) // SLICE st.Ep, $0 | ||
| self.Emit("MOVQ", _DI, _AX) // MOVQ DI, AX | ||
| self.Emit("MOVQ", _SI, _BX) // MOVQ SI, BX | ||
| self.Emit("LEAQ", _VAR_st_Dv, _CX) // LEAQ st.Dv, CX | ||
| self.call_go(_F_decodeFloat32Std) // CALL_GO decodeFloat32Std | ||
| self.Emit("TESTQ", _ET, _ET) // TESTQ ET, ET |
There was a problem hiding this comment.
_asm_OP_map_key_f32 also does a redundant parse (native parse_number + strconv.ParseFloat via decodeFloat32Std). Consider an approach that only scans for the end of the numeric key and parses once with stdlib float32 semantics to reduce overhead for float32 map keys.
| func TestMain(m *testing.M) { | ||
| go func () { | ||
| if !debugAsyncGC { | ||
| return | ||
| } | ||
| println("Begin GC looping...") | ||
| for { | ||
| runtime.GC() | ||
| debug.FreeOSMemory() | ||
| } | ||
| println("stop GC looping!") | ||
| }() | ||
| time.Sleep(time.Millisecond) | ||
| m.Run() | ||
| go func() { | ||
| if !debugAsyncGC { | ||
| return | ||
| } | ||
| println("Begin GC looping...") | ||
| for { | ||
| runtime.GC() | ||
| debug.FreeOSMemory() | ||
| } | ||
| println("stop GC looping!") | ||
| }() | ||
| time.Sleep(time.Millisecond) | ||
| m.Run() | ||
| } |
There was a problem hiding this comment.
TestMain calls m.Run() but does not propagate the exit code. In Go, TestMain should typically terminate with os.Exit(m.Run()) so test failures produce a non-zero process exit status (and CI correctly fails).
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Use skip_number to locate float32 token boundaries and parse once with stdlib semantics in JIT f32 value/map-key paths. Also make TestMain return m.Run() exit code for reliable CI status.
|
|
||
| func (self *_Assembler) _asm_OP_f32(_ *_Instr) { | ||
| var pin = "_f32_end_{n}" | ||
| self.parse_number(float32Type, pin, -1) // PARSE NUMBER |
There was a problem hiding this comment.
Maybe silly question, why not parse as float64 with existing JIT native path, cast it as float32, check float64(f32) == f64, if true then use directly then slow path decodeFloat32Std if not?
decodeFloat32Std might allocate a string header as well, could prime with unsafe.Slice derived string so it avoids heap allocation.
What type of PR is this?
Check the PR title.
(Optional) Translate the PR title into Chinese.
(Optional) More detailed description for this PR(en: English/zh: Chinese).
en:
zh(optional):
(Optional) Which issue(s) this PR fixes:
(optional) The PR that updates user documentation: