De-RDF and de-fork subscribe

ASSIGNED
Assigned to

Status

MailNews Core
Backend
ASSIGNED
9 years ago
4 months ago

People

(Reporter: jcranmer, Assigned: jcranmer)

Tracking

(Blocks: 2 bugs)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 340663 [details] [diff] [review]
A general idea

This bug is to de-RDF and de-fork the subscribe dialog.

Attached is a patch which has my work to date. More history can be seen at the kill-rdf repo.
(Assignee)

Comment 1

9 years ago
Comment on attachment 340663 [details] [diff] [review]
A general idea

Oops, wrong diff.
Attachment #340663 - Attachment is obsolete: true
(Assignee)

Comment 2

9 years ago
Created attachment 340664 [details] [diff] [review]
The correct general idea

As the attachment name says...
(Assignee)

Comment 3

9 years ago
Created attachment 341769 [details] [diff] [review]
Patch, version 0.9

This is the final product, modulo some severe threadline issues.
Attachment #340664 - Attachment is obsolete: true
(Assignee)

Comment 4

9 years ago
Created attachment 341776 [details] [diff] [review]
Patch, version 1

Finally, I have something that works to the point of reviewability.

Before balking at the size, keep a few things in mind:
1. A total of six files are being removed, comprising in sum 2637 lines.
2. This essentially rewrites much of subscribe.js and all useful content of nsISubscribableServer.js.

The changes to subscribe.xul may be better understood by comparing the new file with the old TB version.

In general, the basic model changes from having the interface know the state to having the interface tell the dialog its state. The dialog also rocks much better with async connections (try running it on a server with a large amount of newsgroups on a really slow connection and note that it is usable while getting newsgroups!). The only problem is that large sync loads are more painful. In particular, loading the news host file wants to be done on another thread.

There is one thing which this patch will regress (acceptably, IMHO): selecting servers to subscribe to in search mode and then exiting search mode will "reset" those subscriptions until you go back into search mode. I plan on redoing the search + subscribe feature before TB 3 anyways (asking the server to implement nsITreeView and keep copies of stuff itself is a waste)...

This is more than what is available at the tip of jminta's kill-rdf repo, since I intend to have this land separately, as it does not affect any other kill-rdf work (asides from showing a folderLookupService regression).
Attachment #341769 - Attachment is obsolete: true
Attachment #341776 - Flags: superreview?(neil)
Attachment #341776 - Flags: review?(bienvenu)

Comment 5

9 years ago
(In reply to comment #4)
> The dialog also rocks much better with async connections (try running
> it on a server with a large amount of newsgroups on a really slow
> connection and note that it is usable while getting newsgroups!). The
> only problem is that large sync loads are more painful. In particular,
> loading the news host file wants to be done on another thread.
My ISP's news server carries over 85000 groups, and loading them took an age and there were several slow script warning prompts along the way. By comparison, without the patch the subscribe dialog loaded instantaneously.

Comment 6

9 years ago
Comment on attachment 341776 [details] [diff] [review]
Patch, version 1

Also the themeing changes don't seem to work quite right - the unsubscribable items had a dot in the subscribe column.
Attachment #341776 - Flags: superreview?(neil) → superreview-
(Assignee)

Comment 7

9 years ago
< Side note: I swear I dreamed something about writing a response to this comment a while back... >

(In reply to comment #5)
> My ISP's news server carries over 85000 groups, and loading them took an age
> and there were several slow script warning prompts along the way. By
> comparison, without the patch the subscribe dialog loaded instantaneously.

It sounds like this patch would want to shunt the host file loading off the main thread, then, instead of delaying it to another patch.

(In reply to comment #6)
> Also the themeing changes don't seem to work quite right - the unsubscribable
> items had a dot in the subscribe column.

Which theme is this? I checked my changes on the default theme for Linux on TB (qute) and SM (classic, I think), and both seemed to work...

Comment 8

9 years ago
loading hostinfo.dat is probably a much more common operation than building up the list of newsgroups, so making it slower seems like a move in the wrong direction. And moving it onto its own thread sounds like a drastic work-around for an underlying problem that might be more easily fixed by just using timeouts and loading the hostinfo.dat file in chunks.
(Assignee)

Comment 9

9 years ago
(In reply to comment #8)
> loading hostinfo.dat is probably a much more common operation than building up
> the list of newsgroups, so making it slower seems like a move in the wrong
> direction. And moving it onto its own thread sounds like a drastic work-around
> for an underlying problem that might be more easily fixed by just using
> timeouts and loading the hostinfo.dat file in chunks.

Actually, LoadHostInfoFile is only called by StartPopulating, so it's a sync call happening on the UI thread only happening when someone opens the subscribe dialog. That said, I forgot about being able to use timeouts instead (which is how nsNNTPProtocol downloads the list).
(Assignee)

Comment 10

9 years ago
Created attachment 342611 [details] [diff] [review]
Patch, version 2

This speeds up the dialog considerably, and makes news host file loading asynchronous. Also, this fixes the SM CSS problem that Neil saw. Finally, this fixed a regression in IMAP i18n folder names. There may be one or two other minor fixes in here, but those are the bigger ones.
Attachment #341776 - Attachment is obsolete: true
Attachment #342611 - Flags: superreview?(neil)
Attachment #342611 - Flags: review?(bienvenu)
Attachment #341776 - Flags: review?(bienvenu)
(Assignee)

Comment 11

9 years ago
Created attachment 342823 [details] [diff] [review]
Patch, version 3

This patch contains a few small fixes and one big fix.

The big fix: news load now loads all files
The small fixes:
* I found the cause of the leak. And I fixed it.
* Expanding a news folder while loading hostinfo doesn't stop loading hostinfo
* A bit more error-tolerant
* Removed a few redundant method calls.
* Moved the chunking to 500 items instead of 1000, as the 500 is less hesitant

I could do more thorough cleanup with respect to the search stuff, but that will come when I make search + subscribe sane, hopefully before b1.
Attachment #342611 - Attachment is obsolete: true
Attachment #342823 - Flags: superreview?(neil)
Attachment #342823 - Flags: review?(bienvenu)
Attachment #342611 - Flags: superreview?(neil)
Attachment #342611 - Flags: review?(bienvenu)
Comment on attachment 342823 [details] [diff] [review]
Patch, version 3

As measured on a 3GHz dual-core processor:
Time to load 47496 groups: 31s (unresponsive)
Time to load in 97 batches: 97.5s (responsive)
Time to load with old code: 5s (unresponsive)

>+    if (gSubscribableServer.subscribeListener != gSubscribeTreeView)
>+      gSubscribableServer.subscribeListener = gSubscribeTreeView;
What's the point of the test?

>+    dump("Stack trace:\n" + ex.stack);
undefined

>+    try {
>+      // We just need to know it's doable; actually setting the server will be
>+      // done later
>+      folder.server.QueryInterface(Components.interfaces.nsISubscribableServer);
>+      gServerURI = folder.server.serverURI;
>+    }
>+    catch (ex) {
>+      //dump("not a subscribable server\n");
>+      gSubscribableServer = null;
>+      gServerURI = null;
>+    }
Should use instanceof instead of try { QueryInterface }
Note that our current UI won't let you open the Subscribe dialog unless you have selected a subscribable server.

>+  StateChanged(name,state);
Nit: might as well add a space after the comma

>         if (inSearchMode)
>           SetStateFromRow(k, state);
>         else {
>-          var rowRes = gSubscribeTree.builderView.getResourceAtIndex(k);
>-          var name = GetRDFProperty(rowRes, "Name");
>-          SetState(name, state);
>+          var column = {id: "subscribedColumn"};
>+          if (gSubscribeTreeView.isEditable(k, column))
>+            gSubscribeTreeView.setCellValue(k, column, "" + state);
Followup to make both views use the same code to set the state?

.+let Ci = Components.interfaces;
...

>+function makeRowElement(name, leafName, level) {
>+  let row = {};
>+  row.name = name;
>+  row.leafName = leafName;
>+  row.parent = null;
>+  row.children = [];
>+  row.open = false;
>+  row.level = level;
>+  return row;
>+}
Consider making this a constructor, rather than a function i.e. call it with new rowElement(...) and move the null/false constants to the prototype.

>+    this.setCellValue(aRow, {id: "subscribedColumn"}, !row.subscribed + "");
Why the + ""? Does xpconnect not get this cast right?

>+    this._batchAdditions.push([aName, aSubscribed, aSubscribable, aOverwrite]);
push(arguments); ?

>+    if (this._batchAdditions.length > 500) {
>= (otherwise you get 501 in a batch...)

>+      for each (var args in this._batchAdditions)
Can't use for ... in with an array.

>+        // Custom binary search that returns where the index would be.
Didn't you adapt the other binary search to do this too?

>+      if (mapIndex == gSubscribeTreeView._rowMap.length)
>+        gSubscribeTreeView._rowMap.push(row);
>+      else
>+        gSubscribeTreeView._rowMap.splice(mapIndex, 0, row);
Don't need to special-case this.

>+      return -low - 1;
IMHO ~x is neater than -x - 1

>+      let row = makeRowElement(fullName, midName, level);
>+      row.subscribed = false;
>+      row.canSubscribe = false;
IMHO these should default to false in the rowElement prototype.

>+          // We have a match! Overwrite only if necessary
>+          if (aOverwrite && (row.subscribed != aSubscribed ||
>+                             row.canSubscribe != aSubscribable)) {
Don't you trust the overwrite flag?

>+        addNode(nameParts, child.children, child, child.open, level + 1);
>+        return;
This tail recursion is a somewhat unusual way of writing a loop...

>+      if (this._tree)
>+        this._tree.beginUpdateBatch();
>+      for each (var args in this._batchAdditions)
>+        this._realAddFolder(args[0], args[1], args[2], args[3]);
>+      this._batchAdditions = [];
>+      if (this._tree)
>+        this._tree.endUpdateBatch();
Nit: duplicated code.

>+  isContainer: function (aIndex) {
>+    return !!this._rowMap[aIndex].children.length;
>+  },
Hmm, how does this work for IMAP, I thought we only populate as necessary.

>+      this._tree.rowCountChanged(aIndex + 1, index - aIndex - 1);
>+      this._tree.invalidateRange(aIndex + 1, index - aIndex - 1);
rowCountChanged does invalidate the added rows! You might want to invalidate the twisty though.

>+      this._tree.rowCountChanged(aIndex + 1,  -(i - (aIndex + 1)));
aIndex + 1 - i

>+      return this._rowMap[aRow].subscribed ? "true" : "false";
xpconnect casting issue again?

>+    var atomSvc = Components.classes["@mozilla.org/atom-service;1"]
>+                            .getService(Components.interfaces.nsIAtomService);
Why not use the global atomService?

+    aProps.AppendElement(atomSvc.getAtom("Subscribable-" + row.canSubscribe));
+
+    if (aCol.id == "nameColumn")
+      aProps.AppendElement(atomSvc.getAtom(this._serverType));
Might it be worth getting these atoms globally?
(It's a pity there aren't predefined properties for uneditable cells.)
Attachment #342823 - Flags: superreview?(neil) → superreview-
(Assignee)

Comment 13

9 years ago
(In reply to comment #12)
> >+    if (gSubscribableServer.subscribeListener != gSubscribeTreeView)
> >+      gSubscribableServer.subscribeListener = gSubscribeTreeView;
> What's the point of the test?
> 
> >+    dump("Stack trace:\n" + ex.stack);
> undefined

It's what I've used for whenever someone throws a JS Error at me...

> >         if (inSearchMode)
> >           SetStateFromRow(k, state);
> >         else {
> >-          var rowRes = gSubscribeTree.builderView.getResourceAtIndex(k);
> >-          var name = GetRDFProperty(rowRes, "Name");
> >-          SetState(name, state);
> >+          var column = {id: "subscribedColumn"};
> >+          if (gSubscribeTreeView.isEditable(k, column))
> >+            gSubscribeTreeView.setCellValue(k, column, "" + state);
> Followup to make both views use the same code to set the state?

That'll be part of the subscribe search cleanup, where I intend to make both search and non-search use gSubscribeTreeView.

> >+    this.setCellValue(aRow, {id: "subscribedColumn"}, !row.subscribed + "");
> Why the + ""? Does xpconnect not get this cast right?

I'm pretty sure that xpconnect did 0/1 for JS bools as strings, but I'll double check.

> >+    if (this._batchAdditions.length > 500) {
> >= (otherwise you get 501 in a batch...)
... oops ...

> >+        // Custom binary search that returns where the index would be.
> Didn't you adapt the other binary search to do this too?

This is a standard binary search extension to return more detailed information instead of -1 (at least, I based this off of Java's Arrays.binarySearch). In this case, however, I don't bother trying to look for it in the list since I already know it's not in there.

> >+      if (mapIndex == gSubscribeTreeView._rowMap.length)
> >+        gSubscribeTreeView._rowMap.push(row);
> >+      else
> >+        gSubscribeTreeView._rowMap.splice(mapIndex, 0, row);
> Don't need to special-case this.

Duly noted.

> >+          // We have a match! Overwrite only if necessary
> >+          if (aOverwrite && (row.subscribed != aSubscribed ||
> >+                             row.canSubscribe != aSubscribable)) {
> Don't you trust the overwrite flag?

The overwrite only tells us that we /may/ overwrite it, not that we need overwrite it.

> >+  isContainer: function (aIndex) {
> >+    return !!this._rowMap[aIndex].children.length;
> >+  },
> Hmm, how does this work for IMAP, I thought we only populate as necessary.

IMAP looks at everything two levels at a time, so it's really populating grandchildren as we expand the nodes.

> >+    var atomSvc = Components.classes["@mozilla.org/atom-service;1"]
> >+                            .getService(Components.interfaces.nsIAtomService);
> Why not use the global atomService?

Forgot about that as I was rampaging earlier stuff; will do.

> +    aProps.AppendElement(atomSvc.getAtom("Subscribable-" + row.canSubscribe));
> +
> +    if (aCol.id == "nameColumn")
> +      aProps.AppendElement(atomSvc.getAtom(this._serverType));
> Might it be worth getting these atoms globally?

Ditto.
(Assignee)

Comment 14

9 years ago
(In reply to comment #12)
> >+    this.setCellValue(aRow, {id: "subscribedColumn"}, !row.subscribed + "");
> Why the + ""? Does xpconnect not get this cast right?

xpconnect gets the cast right, but I'm calling this via JS invocation, where I'm expecting a string "true" or "false". And true != "true".
Blocks: 455947
(Assignee)

Comment 15

9 years ago
Created attachment 344841 [details] [diff] [review]
Patch, version 4

Improved patch. Also fixed regression that made news not show full names.
Attachment #342823 - Attachment is obsolete: true
Attachment #344841 - Flags: superreview?(neil)
Attachment #344841 - Flags: review?(bienvenu)
Attachment #342823 - Flags: review?(bienvenu)

Comment 16

8 years ago
where are we with this patch? last I heard, there were still significant performance issues with loading the subscribe UI with it.
(Assignee)

Comment 17

8 years ago
(In reply to comment #16)
> where are we with this patch? last I heard, there were still significant
> performance issues with loading the subscribe UI with it.

That is correct. I had something that was much more performant when I pushed the JS implementation back into C++, but that was a rather ugly implementation. I guess I never got around to clearing review requests...
(Assignee)

Updated

8 years ago
Attachment #344841 - Attachment is obsolete: true
Attachment #344841 - Flags: superreview?(neil)
Attachment #344841 - Flags: review?(bienvenu)
(Assignee)

Comment 18

8 years ago
Comment on attachment 344841 [details] [diff] [review]
Patch, version 4

Canceling requests.
Blocks: 507669
Target Milestone: Thunderbird 3.0b1 → ---
(In reply to Joshua Cranmer [:jcranmer] from comment #4)
> The changes to subscribe.xul may be better understood by comparing the new
> file with the old TB version.

The patch (version 4) has changes to mailnews/base/resources/content/subscribe.js and mailnews/base/resources/content/subscribe.xul.

In current comm-central, there is only mail/base/content/subscribe.js and suite/mailnews/subscribe.js. Apparently, a lot has changed since then. Can someone help how the change to the various subscribe.js and subscribe.xul can be best de-bitrotted?

Joshua, maybe you still have "the old TB version" against which one should compare?

Best regards,

Jens
You need to log in before you can comment on or make changes to this bug.