457333 - De-RDF and de-fork subscribe

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Joshua Cranmer [:jcranmer]]

Attachment: 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.

Comment 1

[Joshua Cranmer [:jcranmer]]

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

Oops, wrong diff.

Comment 2

[Joshua Cranmer [:jcranmer]]

Attachment: The correct general idea

As the attachment name says...

Comment 3

[Joshua Cranmer [:jcranmer]]

Attachment: Patch, version 0.9

This is the final product, modulo some severe threadline issues.

Comment 4

[Joshua Cranmer [:jcranmer]]

Attachment: 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).

Comment 5

[[e-mail]]

(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

[[e-mail]]

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.

Comment 7

[Joshua Cranmer [:jcranmer]]

< 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

[David :Bienvenu]

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.

Comment 9

[Joshua Cranmer [:jcranmer]]

(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).

Comment 10

[Joshua Cranmer [:jcranmer]]

Attachment: 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.

Comment 11

[Joshua Cranmer [:jcranmer]]

Attachment: 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.

Comment 12

[[e-mail]]

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.)

Comment 13

[Joshua Cranmer [:jcranmer]]

(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.

Comment 14

[Joshua Cranmer [:jcranmer]]

(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".

Comment 15

[Joshua Cranmer [:jcranmer]]

Attachment: Patch, version 4

Improved patch. Also fixed regression that made news not show full names.

Comment 16

[David :Bienvenu]

where are we with this patch? last I heard, there were still significant performance issues with loading the subscribe UI with it.

Comment 17

[Joshua Cranmer [:jcranmer]]

(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...

Comment 18

[Joshua Cranmer [:jcranmer]]

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

Canceling requests.

Comment 19

[Jens Müller (:tessarakt)]

(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

Comment 20

[:aceman]

Attachment: 457333-removeSubscribeDS.patch [landed in bug 1495101]

TB and SM subscribe dialog was merged in bug 1425962, made to work without the RDF backend and <template> XUL element.

It seems we can now remove the whole nsSubscribeDatasource files and cut some datasource references from nsSubscribableServer.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=30c8d6588f0de357a3373f34b492d1589c33fb63

Comment 21

[:aceman]

I have aggressively removed GetChildren() method from the SubscribableServer as it seem unused now. It may be it will be needed for the tree implementation. But it will need a different non-RDF reimplamentation (you have something already in your derdf-patches) so then I can put it back.

Comment 22

[:aceman]

Attachment: 457333-1.patch

This salvages whatever is still needed from jcranmer's patches 'subscribedefork' and 'subscribe-fatrip'. Most of those patches are already in the tree due to the quick fix we had to implement in bug 1425962.

Comment 23

[:aceman]

The current version of jcranmer's patches is at https://hg.mozilla.org/users/Pidgeot18_gmail.com/patch-queues/file/tip/patches-derdf/ .
After part 1 that I attached we still need to apply the patches named subscribe-invert and subscribe-tree.

Comment 24

[:aceman]

Comment on attachment 8944167 [details] [diff] [review]
457333-removeSubscribeDS.patch [landed in bug 1495101]

This can be done as the last step after the dialog is fully converted.

Comment 25

[Jorg K (CEST = GMT+2)]

Comment on attachment 8982348 [details] [diff] [review]
457333-1.patch

Looks like some boy-scout clean-up with little change.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ecc150ade2f2
Cleanup subscribe dialog code. r=jorgk

Comment 27

[Jorg K (CEST = GMT+2)]

Comment on attachment 8982348 [details] [diff] [review]
457333-1.patch

We need this or else the patch from bug 1466705 doesn't apply.

Comment 28

[Jorg K (CEST = GMT+2)]

TB 60 beta 8 (BETA_60_CONTINUATION branch):
https://hg.mozilla.org/releases/comm-beta/rev/b0a1a743051f736a85129948ef441d9c6d2a51ae

Comment 29

[Jorg K (CEST = GMT+2)]

I think we should apply more patches in another bug since otherwise we get a big version confusion.

Comment 30

[:aceman]

Great and then all the history stays buried here and the bug summary is nothing about what was landed :(

Comment 31

[Jorg K (CEST = GMT+2)]

Well, you can file the follow-up with "Bug 457333 follow-up: ..." and also include this in the commit message.

Landing patches across versions in a single bug becomes messy real soon, and since I have to manage it, it won't happen.

Comment 32

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-esr60/rev/fbafd43c7a86