Opened 12 years ago

Closed 11 years ago

Last modified 11 years ago

#3142 closed defect (fixed)

Use JSON implementations in Python's standard library instead of cjson

Reported by: erikos Owned by: humitos
Priority: Unspecified by Maintainer Milestone:
Component: Sugar Version: Unspecified
Severity: Unspecified Keywords:
Cc: pbrobinson, sascha_silbe, humitos, godiard Distribution/OS: Unspecified
Bug Status: Unconfirmed

Description

The shell has already moved back to use the standard json implementation, toolkit and datastore should follow.

Here an evaluation that performance wise this might be ok: http://www.tablix.org/~avian/blog/archives/2011/07/a_case_for_cjson/

As well, here the revert commit with a bit of explanation:

commit cff9e43527ead20b088a9bfc6bbf12b0827debfd
Author: Daniel Drake <dsd@laptop.org>
Date:   Wed Nov 18 12:17:47 2009 +0000

    Revert "Move to cjson and drop pyjson and simplejson"
    
    This reverts commit ee4535c98ae74347e7072909d49dcf8a5e16ca7b.
    
    cjson has a big bug dealing with slashes, this is a significant
    long-term bug and upstream has not been responsive other than
    acknowledging it. This bug breaks journal entry bundles.
    http://dev.sugarlabs.org/ticket/1553
    
    Thanks to Martin Langhoff for identifying and researching this issue

diff --git a/src/jarabe/journal/expandedentry.py b/src/jarabe/journal/expandedentry.py
index 94d90ed..4463cac 100644
--- a/src/jarabe/journal/expandedentry.py
+++ b/src/jarabe/journal/expandedentry.py
@@ -23,7 +23,7 @@ import hippo
 import cairo
 import gobject
 import gtk
-import cjson
+import json
 
 from sugar.graphics import style
 from sugar.graphics.icon import CanvasIcon
@@ -303,7 +303,9 @@ class ExpandedEntry(hippo.CanvasBox):
 
         if self._metadata.has_key('buddies') and \
                 self._metadata['buddies']:
-            buddies = cjson.decode(self._metadata['buddies']).values()
+            # json cannot read unicode strings
+            buddies_str = self._metadata['buddies'].encode('utf8')
+            buddies = json.read(buddies_str).values()
             vbox.append(BuddyList(buddies))
             return vbox
         else:

Attachments (4)

0001-Use-json-as-included-in-Python-SL-3142.patch (1.4 KB) - added by humitos 12 years ago.
0001-cjson-json-sl-3142.patch (1.1 KB) - added by RafaelOrtiz 12 years ago.
0001-Use-json-as-included-in-Python-SL-3142.2.patch (2.4 KB) - added by humitos 12 years ago.
Speak Activity Patch
0001-Use-json-as-included-in-Python-SL-3142.3.patch (2.3 KB) - added by humitos 12 years ago.
Read Activity patch

Download all attachments as: .zip

Change History (28)

comment:1 Changed 12 years ago by erikos

See as well that in future Fedora versions cjson will not be available anymore: https://bugzilla.redhat.com/show_bug.cgi?id=737150

comment:2 Changed 12 years ago by erikos

  • Cc pbrobinson added

comment:3 Changed 12 years ago by erikos

Looks like there is still a difference between simplejson and json (contained in Python itself), here the simplejson info:

NAME
    simplejson

FILE
    /usr/lib/python2.7/site-packages/simplejson/__init__.py

DESCRIPTION
    JSON (JavaScript Object Notation) <http://json.org> is a subset of
    JavaScript syntax (ECMA-262 3rd edition) used as a lightweight data
    interchange format.
    
    :mod:`simplejson` exposes an API familiar to users of the standard library
    :mod:`marshal` and :mod:`pickle` modules. It is the externally maintained
    version of the :mod:`json` library contained in Python 2.6, but maintains
    compatibility with Python 2.4 and Python 2.5 and (currently) has
    significant performance advantages, even without using the optional C
    extension for speedups.

In a follow up patch bd3fdd73a853d618aa5000ff5b1ce82f196c8b70 on cff9e43527ead20b088a9bfc6bbf12b0827debfd Daniel moved the shell at least back to simplejson.

We should evaluate the situation now once again and then switch all the modules and activities to whatever is the state of art.

comment:4 Changed 12 years ago by manuq

Seems like simplejson replaced the stdlib json in Python 2.6. If we want to be compatible with Python 2.4, we should perform a check and import simplejson instead. They are interchangeable, share the same API.

In Python 2.7, json seems faster for this simple test case:

[manuq@manuq-laptop olpc]$ python /usr/lib/python2.7/timeit.py -s "import json; d=dict(a=3, b=4)" "json.dumps(d)"
10000 loops, best of 3: 49.7 usec per loop

[manuq@manuq-laptop olpc]$ python /usr/lib/python2.7/timeit.py -s "import simplejson as json; d=dict(a=3, b=4)" "json.dumps(d)"
10000 loops, best of 3: 51.5 usec per loop
[manuq@manuq-laptop olpc]$

In Python 2.6, simplejson is a bit faster:

facundo@phenomenux:~$ timeit.py -s "import json; d=dict(a=3, b=4)" "json.dumps(d)"
10000 loops, best of 3: 21.4 usec per loop
facundo@phenomenux:~$ timeit.py -s "import simplejson as json; d=dict(a=3, b=4)" "json.dumps(d)"
100000 loops, best of 3: 9.5 usec per loop

facundo@phenomenux:~$ timeit.py -s "import json; s=json.dumps(dict(a=3, b=4))" "json.loads(s)"
10000 loops, best of 3: 52.9 usec per loop
facundo@phenomenux:~$ timeit.py -s "import simplejson as json; s=json.dumps(dict(a=3, b=4))" "json.loads(s)"
100000 loops, best of 3: 5.71 usec per loop

comment:5 Changed 12 years ago by sascha_silbe

  • Cc sascha_silbe added

Tracking the sugar-datastore part in #3347.

comment:6 follow-up: Changed 12 years ago by dsd

I think we should go with 'json' as included in Python, dropping support of 2.4 (if that even worked anyway), and accepting that Python 2.7 is required to have the highest JSON performance (which is unlikely to make any noticable impact anyway).

comment:7 in reply to: ↑ 6 Changed 12 years ago by erikos

Replying to dsd:

I think we should go with 'json' as included in Python, dropping support of 2.4 (if that even worked anyway), and accepting that Python 2.7 is required to have the highest JSON performance (which is unlikely to make any noticable impact anyway).

Yes, this is what we discussed as well in the development meeting yesterday. I think it is ok for Sugar >= 0.96 to depend on Python >= 2.6.

comment:8 Changed 12 years ago by erikos

  • Milestone changed from Unspecified by Release Team to 0.98

Current situation, we need this to get fixed for Fedora 18:

  • shell [x]
  • toolkit-gtk2 []
  • sugar-datastore []
  • chat []
  • read []
  • other activities?

comment:10 Changed 12 years ago by humitos

  • Cc humitos added
  • Owner changed from erikos to humitos
  • Status changed from new to accepted

comment:11 Changed 12 years ago by humitos

More activities that I found using cjson

  • infoslicer
  • memorize
  • speak

comment:13 Changed 12 years ago by RafaelOrtiz

About memorize and speak.

Speak does not use cjson, memorize is using and older version of speak that it does,
so we should only upgrade the speak part of memorize and thus fixing two activities.

Changed 12 years ago by RafaelOrtiz

comment:14 follow-up: Changed 12 years ago by RafaelOrtiz

Attached a proposed patch for chat.

comment:15 in reply to: ↑ 14 Changed 12 years ago by erikos

Replying to RafaelOrtiz:

Attached a proposed patch for chat.

Looks good to me, added Aleksey to cc.

Changed 12 years ago by humitos

Speak Activity Patch

Changed 12 years ago by humitos

Read Activity patch

comment:17 Changed 12 years ago by godiard

Pushed Read patch as 834444c0c3c712cd5b3d66169e8b5e0230d8ee11

Please, put a reference in the title about the activity involved, and cc in the ticket to the maintainers involved.

comment:18 Changed 12 years ago by humitos

For Typing Turtle Activity, see: #3780

comment:19 Changed 12 years ago by erikos

What is the status on this one? Are we done with this? All activity fixes have been pushed?

comment:20 follow-up: Changed 12 years ago by humitos

  • Cc godiard added

No, at least, the patch for Read was not pushed yet. CCing Gonzalo so he can review this.

comment:21 in reply to: ↑ 20 Changed 12 years ago by humitos

Replying to humitos:

No, at least, the patch for Read was not pushed yet. CCing Gonzalo so he can review this.

Sorry, I didn't see the previous comment. Anyway, I ran

$ git show 834444c0c3c712cd5b3d66169e8b5e0230d8ee11
fatal: bad object 834444c0c3c712cd5b3d66169e8b5e0230d8ee11

Later, I found it here:

http://git.sugarlabs.org/read/mainline/commit/fde52383c637b1d5648d7550a29bb9b1215e896c

Memorize and Speak are waiting...

comment:22 Changed 11 years ago by erikos

There are tickets for the individual activities: #3949 #3950 #3951 #3952

Closing this ticket, datastore, toolkit and shell are done.

comment:23 Changed 11 years ago by erikos

  • Resolution set to fixed
  • Status changed from accepted to closed

comment:24 Changed 11 years ago by dnarvaez

  • Milestone 0.98 deleted

Milestone 0.98 deleted

Note: See TracTickets for help on using tickets.