Ticket #3142 (closed defect: fixed)

Opened 20 months ago

Last modified 8 months ago

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

Reported by: erikos Owned by: humitos
Priority: Unspecified by Maintainer Milestone: 0.98
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

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

Change History

  Changed 20 months 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

  Changed 20 months ago by erikos

  • cc pbrobinson added

  Changed 15 months 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.

  Changed 15 months 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

  Changed 15 months ago by sascha_silbe

  • cc sascha_silbe added

Tracking the sugar-datastore part in #3347.

follow-up: ↓ 7   Changed 15 months 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).

in reply to: ↑ 6   Changed 15 months 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.

  Changed 9 months 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-gtk3 [x]  http://git.sugarlabs.org/sugar-toolkit-gtk3/sugar-toolkit-gtk3/commit/3c39375d9bb59a2bb1c9ec163645d0b4939893ae

- toolkit-gtk2 []

- sugar-datastore []

- browse [x]  http://git.sugarlabs.org/browse/mainline/commit/8b7a3b5cebb8ed7ead60841d052c02753d09ac34

- chat []

- read []

- other activities?

  Changed 9 months ago by humitos

  • cc humitos added
  • owner changed from erikos to humitos
  • status changed from new to accepted

Changed 9 months ago by humitos

  Changed 9 months ago by humitos

More activities that I found using cjson

  • infoslicer
  • memorize
  • speak

  Changed 9 months 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 9 months ago by RafaelOrtiz

follow-up: ↓ 15   Changed 9 months ago by RafaelOrtiz

Attached a proposed patch for chat.

in reply to: ↑ 14   Changed 9 months ago by erikos

Replying to RafaelOrtiz:

Attached a proposed patch for chat.

Looks good to me, added Aleksey to cc.

Changed 9 months ago by humitos

Speak Activity Patch

Changed 9 months ago by humitos

Read Activity patch

  Changed 9 months 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.

  Changed 9 months ago by humitos

For Typing Turtle Activity, see: #3780

  Changed 8 months ago by erikos

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

follow-up: ↓ 21   Changed 8 months ago by humitos

  • cc godiard added

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

in reply to: ↑ 20   Changed 8 months 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...

  Changed 8 months ago by erikos

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

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

  Changed 8 months ago by erikos

  • status changed from accepted to closed
  • resolution set to fixed
Note: See TracTickets for help on using tickets.