Ref T5297. Modify phutil_json_decode to use PhutilJSONParser instead of the native json_decode.
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T5297: Create a native JSON parser.
- Commits
- rPHUaaf34fb75a84: Modify `phutil_json_decode` to use `PhutilJSONParser`.
Ran arc unit.
Diff Detail
Diff Detail
- Repository
- rPHU libphutil
- Branch
- json_decode
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 1260 Build 1260: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
Comment Actions
I think we should still try the native parser first (I'm guessing it's like 1000x faster? Maybe I'm wrong on this) and then use the nice-exceptions parser if it fails, so that we can get a pretty error message without eating the performance hit in the common case.
Let me see if I can substantiate that performance claim...
With a script like this:
#!/usr/bin/env php <?php require_once 'scripts/__init_script__.php'; $data = Filesystem::readFile('../arcanist/resources/php_compat_info.json'); $t_start = microtime(true); $parsed = /* choose a method */ $t_end = microtime(true); print_r($parsed); echo (int)(1000000 * ($t_end - $t_start)).' us'."\n";
..I get:
| Parser | Cost | Relative |
|---|---|---|
| json_decode() | 7,120 us | 1.0x |
| PhutilJSONParser->parse() | 4,760,020 us | 668.5x |
So not quite 1000x faster, but nearly that.
Comment Actions
There's probably no point using PhutilJSONParser here at all then? If we are just going to absorb the exception and return $default.
Comment Actions
I think we should completely replace phutil_json_decode() with some function which:
- uses json_decode() by default;
- falls back to PhutilJSONParser on failure to get a nice exception; and
- always throws on failure.
Basically, offer performant decode of standard JSON with exception-oriented semantics.
That could be accomplished by changing the semantics of phutil_json_decode() to remove $default, or by introducing a different function and deprecating/removing phutil_json_decode().
Since phutil_json_decode() was introduced pretty recently and only has about 10 callsites across everything, changing the semantics seems reasonable to me.
Comment Actions
Alternatively, that could be a new PhutilJSONParser::parseStandardJSON(...) or something.
Comment Actions
- Change the semantics of the phutil_json_decode function.
(We will have to update all callsites as well)