Page MenuHomePhabricator

Modify `phutil_json_decode` to use `PhutilJSONParser`.
ClosedPublic

Authored by joshuaspence on Jun 20 2014, 12:38 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 20, 9:45 PM
Unknown Object (File)
Sat, Mar 16, 2:52 AM
Unknown Object (File)
Sat, Mar 16, 2:04 AM
Unknown Object (File)
Feb 9 2024, 11:07 AM
Unknown Object (File)
Feb 9 2024, 11:07 AM
Unknown Object (File)
Feb 9 2024, 11:07 AM
Unknown Object (File)
Feb 9 2024, 11:06 AM
Unknown Object (File)
Feb 4 2024, 3:19 AM
Subscribers

Details

Summary

Ref T5297. Modify phutil_json_decode to use PhutilJSONParser instead of the native json_decode.

Test Plan

Ran arc unit.

Diff Detail

Repository
rPHU libphutil
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

joshuaspence retitled this revision from to Modify `phutil_json_decode` to use `PhutilJSONParser`..
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.
epriestley edited edge metadata.

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:

ParserCostRelative
json_decode()7,120 us1.0x
PhutilJSONParser->parse()4,760,020 us668.5x

So not quite 1000x faster, but nearly that.

This revision now requires changes to proceed.Jun 20 2014, 1:49 AM

There's probably no point using PhutilJSONParser here at all then? If we are just going to absorb the exception and return $default.

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.

Alternatively, that could be a new PhutilJSONParser::parseStandardJSON(...) or something.

joshuaspence edited edge metadata.
  • Change the semantics of the phutil_json_decode function.

(We will have to update all callsites as well)

epriestley edited edge metadata.
This revision is now accepted and ready to land.Jun 20 2014, 12:37 PM