Closed Bug 380839 (ArrayConverter) Opened 13 years ago Closed 2 years ago

Code-sharing: Implement JS array conversion utility (nsIArray, nsISimpleEnumerator)

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: WeirdAl, Unassigned)

References

Details

(Keywords: memory-footprint)

Attachments

(2 files, 1 obsolete file)

Dealing with arrays in JS-based components requires a bit of work.  I'd like to implement a wrapper object to convert arrays from JavaScript native arrays to nsIArray objects, nsISimpleEnumerator objects, etc., and vice versa.

Suggested API:

ArrayConverter = {
  /**
   * Get a JavaScript array for a nsIArray or nsISimpleEnumerator.
   *
   * @throws NS_ERROR_INVALID_ARG if aObject isn't an nsIArray or nsISimpleEnumerator.
   */
  JSArray: function getJSArray(/* in nsIArray or nsISimpleEnumerator */ aObject) {
  },

  /**
   * Return a nsIArray for a JavaScript array.
   */
  nsIArray: function get_nsIArray(/* in JSArray */ aArray) {
  },

  /**
   * Return a nsISimpleEnumerator for a JavaScript array.
   */
  enumerator: function get_enumerator(/* in JSArray */ aArray) {
  },

  xpcomArrayMethod: function getXPCOMMethod(/* in JSString */ aArrayName) {
    return function xpcomArray(aCount) {
      var array = this[aArrayName];
      aCount.value = array.length;
      return array;
    }
  }
}

Primary question:  shoudl this go into XPCOMUtils.jsm, or into a new file?
Attached patch patch, v1 (obsolete) — Splinter Review
Like this, maybe.

sayrer, who'd be the right people to r/sr on this?  The code-sharing base is so new.
It's XPConnect, pick your favorite peer or something.

Comment on attachment 264982 [details] [diff] [review]
patch, v1

that would be timeless - if he and shaver agree on something, it must be good ;)
Attachment #264982 - Flags: review?(timeless)
I posted this as a comment on Alex's blog, and he asked me to file it here.

Along with an array to nsISimpleEnumerator code, could we get one for returning an nsIStringEnumerator of a javascript object's property names? In our forecastfox extension we had to create a helper function to do this like below:

/******************************************************************************
 * String enumerator of hash table keys.
 * 
 * @param   Javascript hash table. 
 * @return  A nsIStringEnumerator of the keys.
 *****************************************************************************/
function KeyEnumerator(aHashTable)
{
  //setup key array
  this._keys = [];
  this._index = 0;
  
  //load with data
  if (aHashTable) {
    for (var name in aHashTable)
      this._keys.push(name);
  }
}
KeyEnumerator.prototype = {
  _index: null,
  _keys: null,
  
  QueryInterface: function KeyEnumerator_QueryInterface(aIID)
  {
    if (!aIID.equals(Ci.nsIStringEnumerator) ||
        !aIID.equals(Ci.nsISupports))
      throw Cr.NS_ERROR_NO_INTERFACE; 
    return this;   
  },
  
  hasMore: function KeyEnumerator_hasMore()
  {
    return this._index < this._keys.length;
  },
  
  getNext: function KeyEnumerator_getNext()
  {
    var rv = this._keys[this._index];
    this._index++; 
    return rv;
  }
};
[10:59]	<timeless>	var rv = [];
[10:59]	<timeless>	imo rv should only be nsresult
[11:00]	<timeless>	why not use ary.push() ?
[11:00]	<WeirdAl>	years ago, I tried that, and I found that push was slower than ary[ary.length]
[11:00]	<timeless>	interesting
[11:03]	<timeless>	 getPropertyAsInt32: NOT_IMPLEMENTED
[11:03]	<timeless>	does that really do the right thing?
[11:04]	<WeirdAl>	there, I was thinking that this test component does NOT want to implement everything
[11:04]	<WeirdAl>	it's a test component

[11:24]	<WeirdAl>	hm, I could've combined the |catch (e if (e instance of nsIException)) { if (e.result == NS_ERROR_FAILURE)) {}}| lines

[11:46]	<timeless>	 JSArray: function getJSArray(aObject) {
[11:46]	<timeless>	if (aObject instanceof Components.interfaces.nsIArray) {
[11:46]	<timeless>	return this.JSArray(aObject.enumerate());
[11:46]	<timeless>	}
[11:46]	<timeless>	
[11:46]	<timeless>	if (!(aObject instanceof Components.interfaces.nsISimpleEnumerator)) {
[11:46]	<timeless>	you're actually recursing in that first case?
[11:46]	<timeless>	why not:
[11:46]	<timeless>	aObject = aObject.enumerate();
[11:47]	<timeless>	and just let the code continue w/o recursing
[11:47]	<WeirdAl>	I'd thought redeclaring aObject was a strict warning
[11:47]	<timeless>	who said anything about redeclaring?
[11:47]	<WeirdAl>	though I could do var obj = aObj.enumerate()
[11:47]	<timeless>	int c_function (int q) {
[11:47]	<timeless>	q = q + 1;
[11:47]	<timeless>	return q;
[11:47]	<timeless>	}
[11:48]	<WeirdAl>	I've seen strict warnings for redeclarations of function args in JS
[11:48]	<timeless>	you're allowed to recycle arguments
[11:48]	<timeless>	as long as you don't actually declare it.
[11:48]	<timeless>	because it *is* declared
[11:48]	<timeless>	you're just changing its value :)
[11:49]	<WeirdAl>	hehe, and of course that doesn't propagate out
[11:49]	<timeless>	of course :)
[11:52]	<timeless>	function NOT_IMPLEMENTED() {
[11:52]	<timeless>	could you rename that func_NOT_IMPLEMENTED or something

[11:58]	<timeless>	 var bag = Components.classes[contractID]
[11:58]	<timeless>	                     .createInstance(nsIPropertyBag2);
[11:58]	<timeless>	do_check_true(Boolean(bag));
[11:58]	<timeless>	that check seems useless
[11:58]	<timeless>	since cI will throw, no?
[11:59]	<WeirdAl>	I can drop it

[11:59]	<timeless>	DIRS +=  loader DIRS  += loader jscodelib
[11:59]	<timeless>	imo DIRS and friends should be multiline
[12:00]	<timeless>	topsrcdir= @top_srcdir@
[12:00]	<timeless>	srcdir= @srcdir@
[12:00]	<timeless>	topsrcdir is written differently from the others
[12:00]	<timeless>	i think bsmedberg recommends not using tabs for this junk in new files

[12:00]	<timeless>	 * The Original Code is Code modules: JavaScript array converter.
[12:00]	<timeless>	why two spaces after modules:?

[12:01]	<WeirdAl>	ok, what about Richard Klein's comment 4; do you want a string enumerator implemented as he suggests?
[12:02]	<WeirdAl>	he just wants to know if the patch should include support for making nsIStringEnumerator objects
[12:02]	<timeless>	i'm not opposed
[12:03]	<timeless>	however i don't consider it blocking
[12:03]	<WeirdAl>	neither am I; it's just an interface I've never used.
[12:03]	<timeless>	i think it's probably better to get one round into cvs and do a new interface as a new bug for the same file
[12:03]	<WeirdAl>	ok

(Sorry, Richard.  We'll do it later.)
Attached patch patch, v1.1Splinter Review
updated to reflect timeless's comments
Attachment #264982 - Attachment is obsolete: true
Attachment #266436 - Flags: review?(timeless)
Attachment #264982 - Flags: review?(timeless)
Attachment #266436 - Flags: review?(timeless) → review+
Attachment #266436 - Flags: superreview?(shaver)
Comment on attachment 266436 [details] [diff] [review]
patch, v1.1

A few minor changes are necessary to match changes implemented in bug 380970.  Shaver, do you want me to submit a new patch?

>Index: js/src/xpconnect/jscodelib/ArrayConverter.jsm
>+ * Import into a JS component using
>+ * 'Components.utils.import("rel:ArrayConverter.jsm");'

'Components.utils.import("resource://res/modules/ArrayConverter.jsm");

>Index: js/src/xpconnect/jscodelib/Makefile.in
>+EXTRA_COMPONENTS = ArrayConverter.jsm

Now EXTRA_JS_MODULES.

>Index: js/src/xpconnect/tests/unit/component_bug380839.js

>+Components.utils.import("rel:XPCOMUtils.jsm");
>+Components.utils.import("rel:ArrayConverter.jsm");

Again, resource://res/modules/...

>+var NSGetModule = XPCOMUtils.generateNSGetModule([
>+  {
>+    className:  PropertyBag.prototype.classDescription,
>+    cid:        PropertyBag.prototype.classID,
>+    contractID: PropertyBag.prototype.contractID,
>+    factory: XPCOMUtils.generateFactory(
>+      PropertyBag,
>+      PropertyBag.prototype._interfaces
>+    )
>+  }
>+], null, null);

I don't remember, but this may have changed as well.
<shaver> I'll delegate my review to sayrer, if he wants it
<WeirdAl> shaver: as a sr?
<shaver> yeah, I'll sr+ if he says I should
<sayrer> I don't think we should take things we don't use
<sayrer> (cool as it looks)
<WeirdAl> sayrer: in other words, someone should use it right away, huh?
<shaver> oh, there's no caller added?
<sayrer> yes, that way we know if it reduces code
<shaver> I'd be OK taking it if we just had a list of targets
<shaver> even if we don't convert them on the same pass, the list could be compelling enough
<WeirdAl> shaver: I'll gladly generate that list.
<vlad> would probalby be good to convert at least one

Per the above, I'll be submitting a new patch which uses this code tonight, and I'll also generate a list of low-hanging-fruit code that could use this.
Here's a quick list of JS files that could reduce a few lines from using the ArrayConverter:

http://mxr.mozilla.org/seamonkey/source/netwerk/test/httpserver/httpd.js
http://mxr.mozilla.org/seamonkey/source/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
http://mxr.mozilla.org/seamonkey/source/browser/components/microsummaries/src/nsMicrosummaryService.js (coming with the next patch)
http://mxr.mozilla.org/seamonkey/source/chrome/test/unit/test_bug380398.js
http://mxr.mozilla.org/seamonkey/source/directory/xpcom/datasource/nsLDAPDataSource.js
http://mxr.mozilla.org/seamonkey/source/calendar/providers/composite/calCompositeCalendar.js (maybe)
http://mxr.mozilla.org/seamonkey/source/browser/components/bookmarks/src/nsBookmarkTransactionManager.js
http://mxr.mozilla.org/seamonkey/source/browser/components/feeds/src/FeedWriter.js
http://mxr.mozilla.org/seamonkey/source/browser/components/feeds/src/WebContentConverter.js
http://mxr.mozilla.org/seamonkey/source/browser/components/sidebar/src/nsSidebar.js
http://mxr.mozilla.org/seamonkey/source/browser/fuel/src/fuelApplication.js
http://mxr.mozilla.org/seamonkey/source/content/xslt/src/xslt/txEXSLTRegExFunctions.js
http://mxr.mozilla.org/seamonkey/source/content/xtf/test/unit/xtfComponent.js
http://mxr.mozilla.org/seamonkey/source/calendar/base/src/calRecurrenceInfo.js
http://mxr.mozilla.org/seamonkey/source/calendar/base/src/calAlarmService.js
http://mxr.mozilla.org/seamonkey/source/calendar/base/src/calTodo.js
http://mxr.mozilla.org/seamonkey/source/calendar/base/src/calCalendarManager.js
http://mxr.mozilla.org/seamonkey/source/calendar/base/src/calItipProcessor.js
http://mxr.mozilla.org/seamonkey/source/calendar/base/src/calAttachment.js
http://mxr.mozilla.org/seamonkey/source/calendar/base/src/calEvent.js
http://mxr.mozilla.org/seamonkey/source/calendar/base/src/calItipItem.js
http://mxr.mozilla.org/seamonkey/source/calendar/base/src/calAttendee.js
http://mxr.mozilla.org/seamonkey/source/calendar/providers/wcap/calWcapCachedCalendar.js
http://mxr.mozilla.org/seamonkey/source/calendar/providers/wcap/calWcapSession.js
http://mxr.mozilla.org/seamonkey/source/calendar/providers/wcap/calWcapCalendar.js
http://mxr.mozilla.org/seamonkey/source/calendar/providers/gdata/components/calGoogleCalendarModule.js

Also, any JS component implementing one of the interfaces listed here:
http://mxr.mozilla.org/seamonkey/search?string=size_is&find=%5C.idl%24&findi=&filter=&tree=seamonkey

Example:
http://mxr.mozilla.org/seamonkey/source/browser/components/search/nsSearchService.js
Status: NEW → ASSIGNED
Attached patch patch, v1.2Splinter Review
timeless has already r+'d this patch; this follow-on review request is for shaver's benefit.
Attachment #269203 - Flags: review?(sayrer)
Comment on attachment 269203 [details] [diff] [review]
patch, v1.2

>Index: browser/components/microsummaries/src/nsMicrosummaryService.js
>   getBookmarks: function MSS_getBookmarks() {
>-    return new ArrayEnumerator(this._getBookmarks());
>+    return ArrayConverter.nsIArray(this._getBookmarks());
>   },

>   Enumerate: function MSSet_Enumerate() {
>-    return new ArrayEnumerator(this._elements);
>+    return ArrayConverter.nsIArray(this._elements);
>   }
> };

Whoops!!  Major mea culpa here; that should've been ArrayConverter.enumerator(this._getBookmarks()), ArrayConverter.enumerator(this._elements),
Alias: ArrayConverter
Attachment #266436 - Flags: superreview?(shaver)
I think that the method names should follow usual conventions, so it would be for example ArrayConverter.toJSArray, ArrayConverter.toIArray, ArrayConverter.toEnumerator. And while I really cannot figure out what ArrayConverter.xpcomArrayMethod is good for, the better name for it is probably ArrayConverter.getXPCOMArrayMethod.
Comment on attachment 269203 [details] [diff] [review]
patch, v1.2

>+    var type = this._itemsList[this._iteratorPosition];
>+    this._iteratorPosition++;
>+    return type;
What's wrong with
return this._itemsList[this._iteratorPosition++];

>+    if (aIndex > this.length - 1) {
aIndex >= this.length

>+    return this._itemsList[aIndex].QueryInterface(aIID);

>+  indexOf: function indexOf(aIndex, aElement) {
>+    for (var i = aIndex; i < this._itemsList.length; i++) {
>+      if (this._itemsList[i] == aElement) {
>+        return i;
Can'r you use _itemsList.indexOf(aElement, aIndex) somehow?

>+      array[array.length] = aObject.getNext();
array.push

>+      var rv = [];
>+      for (var i = 0; i < array.length; i++) {
>+        rv[i] = array[i];
>+      }
>+      return rv;
No need to manually clone the array, just return it.
>>+      array[array.length] = aObject.getNext();
>array.push
I'm told push sucks :-(
Told by whom?  push used to suck, relative to that pattern, but we fixed it.  Unless you can demonstrate a performance problem, prefer clarity.
Neil: Thanks for the comments; when I wrote this patch I hadn't yet started using some of the JS1.6+ features.

Also, the reason I haven't continued to drive this is I was under the impression it wouldn't make 1.9.
The work to speed up array.push was for bug 385393. Economics shifted with the introduction of JSOP_LENGTH and inline special casing in JSOP_SETELEM, though, so it's not for sure that a.push is as fast or faster than a[a.length] = b -- but it pays to remember Djikstra's "premature optimization is the root of all evil".

Still, re-benchmarking would be interesting. If push is a lot slower again, please let me know (mail is fine). We might want a new bug, but data first.

/be
Fwiw, see also bug 418490 /mailnews/base/util/iteratorUtils.jsm.
Depends on: 418490
Version: unspecified → Trunk
Assignee: ajvincent → nobody
What's the status of this. If I integrate Neil's comments and post a new patch, is there anything else preventing it from being reviewed/committed?

Robert, are you still the guy to review this?
Matthew, I've come to rethink a big part of this patch.  See http://weblogs.mozillazine.org/weirdal/archives/019648.html - we should use the native nsIMutableArray implementation.
Alex, are you suggesting that we instantiated an @mozilla.org/array;1 and then copy all the elements from the JS array into it? If so, this seems like it could be very memory-inefficient for a large array (and might incur a significant performance penalty when the array is created). I'm not convinced the current approach is not better. If people want to use @mozilla.org/array;1 in their code, it's not much of a burden for them to do that by hand.

I'm interested in opinions.
Status: ASSIGNED → NEW
Keywords: footprint
Attachment #269203 - Flags: review?(sayrer)
It doesn't feel like we'd bother doing something like this at this point.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.