Closed Bug 922190 Opened 6 years ago Closed 6 years ago

remove bundled copy of simplejson

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla27

People

(Reporter: Benjamin, Assigned: Benjamin)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch remove bundled simplejson (obsolete) — Splinter Review
Python 2.7 is required to build the tree and Python 2.6+ include simplejson in the stdlib (as json), There's not much point in having a copy of simplejson sitting around in the tree when the stdlib module can just be used.

Removing python/simplejson will bring us down to one copy of simplejson in the addon sdk...

I'm attaching a patch which removes the bundled simplejson and changes all imports of simplejson to just json.
Attachment #812117 - Flags: review?(gps)
Comment on attachment 812117 [details] [diff] [review]
remove bundled simplejson

Review of attachment 812117 [details] [diff] [review]:
-----------------------------------------------------------------

This patch makes me happy. However, code in testing/mozbase is canonically managed at https://github.com/mozilla/mozbase. In theory, changes are supposed to land there first then merge into m-c. mozbase may also have other requirements for supporting older Python versions - I can't recall what they're up to these days.

Anyway, I'm happy with everything outside of testing/mozbase. But I'm going to hold off on r+ until jhammel chimes in with mozbase knowledge.
Attachment #812117 - Flags: feedback?(jhammel)
You also forgot a reference in build/virtualenv_packages.txt.
Comment on attachment 812117 [details] [diff] [review]
remove bundled simplejson

Yes, please ticket the mozbase issue and submit a patch vs the github repo as according to https://wiki.mozilla.org/Auto-tools/Projects/Mozbase . Since all of the simplejson imports are optional, this should not block the work herein
Attachment #812117 - Flags: feedback?(jhammel) → feedback-
Here's the patch with build/virtualenv_packages.txt fixed (I'm surprised the build didn't fail without that change) and without the mozbase changes.
Attachment #812117 - Attachment is obsolete: true
Attachment #812117 - Flags: review?(gps)
Attachment #812183 - Flags: review?(gps)
The build didn't fail without the build/virtualenv_packages.txt change because we use .pth files and the code that creates them isn't smart enough to check the dest path exists before installing them. I hope to one day have proper packaging with proper dependencies. Our Python environment management is so lacking...
I just want to mention that simplejson != json for Python > 2.6. Simplejson is still a separate developed module with speed improvements and more up-to-date code. The json package shipped with Python is a bit outdated.

If we don't need the speed improvements by the compiled code we could remove it. But I think we should do some perf analysis that we don't regress something for tools which have a high usage of simplejson.
It's true that simplejson != json, but the stdlib json module has also seen optimization work by core CPython developers. I've never actually seen hard evidence that simplejson is significantly faster.
Comment on attachment 812183 [details] [diff] [review]
remove bundled simplejson

Review of attachment 812183 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not too concerned about json perf issues. I don't believe there's any Python system currently generating enough JSON for this to be an issue. Although, the structured logging stuff being used could test that.

If we're really concerned about json perf, I'd be more inclined to use https://pypi.python.org/pypi/ujson, as it seems to outperform everything.
Attachment #812183 - Flags: review?(gps) → review+
https://hg.mozilla.org/mozilla-central/rev/2786f006654b
Assignee: nobody → benjamin
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.