7804 dataverse page speedup

doc/sphinx-guides/source/developers/configuration.rst

@@ -23,7 +23,7 @@ Developers have accessed the simple properties via
 1. ``System.getProperty(...)`` for JVM system property settings
 2. ``SettingsServiceBean.get(...)`` for database settings and
 3. ``SystemConfig.xxx()`` for specially treated settings, maybe mixed from 1 and 2 and other sources.
-4. ``SettingsWrapper``, reading from 2 and 3 for use in frontend pages based on JSF
+4. ``SettingsWrapper`` must be used to obtain settings from 2 and 3 in frontend JSF (xhtml) pages. Please see the note on how to :ref:`avoid common efficiency issues with JSF render logic expressions <avoid-efficiency-issues-with-render-logic-expressions>`.
 
 As of Dataverse Software 5.3, we start to streamline our efforts into using a more consistent approach, also bringing joy and
 happiness to all the system administrators out there. This will be done by adopting the use of

doc/sphinx-guides/source/developers/tips.rst

@@ -177,6 +177,43 @@ If you already have a working dev environment with Glassfish and want to switch
 
 - Copy the "domain1" directory from Glassfish to Payara.
 
+UI Pages Development
+--------------------
+
+While most of the information in this guide focuses on service and backing beans ("the back end") development in Java, working on JSF/Primefaces xhtml pages presents its own unique challenges. 
+
+.. _avoid-efficiency-issues-with-render-logic-expressions:
+
+Avoiding Inefficiencies in JSF render logic
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+It is important to keep in mind that the expressions in JSF ``rendered=`` attributes may be evaluated **multiple** times. So it is crucial not to use any expressions that require database lookups, or otherwise take any appreciable amount of time and resources. Render attributes should exclusively contain calls to methods in backing beans or caching service wrappers that perform any real work on the first call only, then keep returning the cached result on all the consecutive calls. This way it is irrelevant how many times PrimeFaces may need to call the method as any effect on the performance will be negligible.
+
+If you are ever in doubt as to how many times the method in your render logic expression is called, you can simply add a logging statement to the method in question. Or you can simply err on the side of assuming that it's going to be called a lot, and ensure that any repeated calls are not expensive to process.
+
+A simplest, trivial example would be a direct call to a method in SystemConfig service bean. For example, 
+
+``<h:outputText rendered="#{systemConfig.advancedModeEnabled}" ...``
+
+If this method (``public boolean isAdvancedModeEnabled()`` in ``SystemConfig.java``) consults a database setting every time it is called, this database query will be repeated every time JSF reevaluates the expression above. A lookup of a single database setting is not very expensive of course, but repeated enough times unnecessary queries do add up, especially on a busy server. So instead of SystemConfig, SettingsWrapper (a ViewScope bean) should be used to cache the result on the first call:
+
+``<h:outputText rendered="#{settingsWrapper.advancedModeEnabled}" ...``
+
+with the following code in ``SettingsWrapper.java``:
+
+.. code:: java
+
+	  private Boolean  advancedModeEnabled = null; 
+
+	  public boolean isAdvancedModeEnabled() {
+	     if (advancedModeEnabled == null) {
+                advancedModeEnabled = systemConfig.isAdvancedModeEnabled();
+             }
+             return advancedModeEnabled; 
+          }
+
+A more serious example would be direct calls to PermissionServiceBean methods used in render logic expressions. This is something that has happened and caused some problems in real life. A simple permission service lookup (for example, whether a user is authorized to create a dataset in the current dataverse) can easily take 15 database queries. Repeated multiple times, this can quickly become a measurable delay in rendering the page. PermissionsWrapper must be used exclusively for any such lookups from JSF pages.
+
 ----
 
 Previous: :doc:`dev-environment` | Next: :doc:`troubleshooting`

src/main/java/edu/harvard/iq/dataverse/DatasetPage.java

@@ -324,8 +324,6 @@ public void setSelectedHostDataverse(Dataverse selectedHostDataverse) {
 
     private Boolean hasTabular = false;
 
-    //External Vocabulary support
-    Map<Long, JsonObject> cachedCvocMap=null;
 
     /**
      * If the dataset version has at least one tabular file. The "hasTabular"
@@ -5532,16 +5530,8 @@ public String getLocaleDisplayName(String code) {
         return displayName; 
     }
 
-    public Map<Long, JsonObject> getCVocConf() {
-        //Cache this in the view
-        if(cachedCvocMap==null) {
-        cachedCvocMap = fieldService.getCVocConf(false);
-        }
-        return cachedCvocMap;
-    }
-
     public List<String> getVocabScripts() {
-        return fieldService.getVocabScripts(getCVocConf());
+        return fieldService.getVocabScripts(settingsWrapper.getCVocConf());
     }
 
     public String getFieldLanguage(String languages) {

src/main/java/edu/harvard/iq/dataverse/DataverseHeaderFragment.java

@@ -9,7 +9,6 @@
 import edu.harvard.iq.dataverse.authorization.users.AuthenticatedUser;
 import edu.harvard.iq.dataverse.authorization.users.User;
 import edu.harvard.iq.dataverse.settings.SettingsServiceBean;
-import static edu.harvard.iq.dataverse.util.JsfHelper.JH;
 
 import edu.harvard.iq.dataverse.util.BundleUtil;
 import edu.harvard.iq.dataverse.util.SystemConfig;
@@ -40,15 +39,9 @@ public class DataverseHeaderFragment implements java.io.Serializable {
     @EJB
     DataverseServiceBean dataverseService;
 
-    @EJB
-    SettingsServiceBean settingsService;
-
     @EJB
     GroupServiceBean groupService;
 
-    @EJB
-    PermissionServiceBean permissionService;
-
     @EJB
     SystemConfig systemConfig;
 
@@ -75,7 +68,7 @@ public class DataverseHeaderFragment implements java.io.Serializable {
 
     List<Breadcrumb> breadcrumbs = new ArrayList<>();
 
-    private List<BannerMessage> bannerMessages = new ArrayList<>();
+    private List<BannerMessage> bannerMessages = null; 
 
     private Long unreadNotificationCount = null;
 
@@ -161,7 +154,7 @@ public Long getUnreadNotificationCount(Long userId){
             this.unreadNotificationCount = userNotificationService.getUnreadNotificationCountByUser(userId);
         }catch (Exception e){
             logger.warning("Error trying to retrieve unread notification count for user." + e.getMessage());
-            this.unreadNotificationCount = new Long("0");
+            this.unreadNotificationCount = 0L; 
         }
         return this.unreadNotificationCount;
     }
@@ -261,7 +254,7 @@ public String logout() {
     private Boolean signupAllowed = null;
 
     private String redirectToRoot(){
-        return "dataverse.xhtml?alias=" + dataverseService.findRootDataverse().getAlias();
+        return "dataverse.xhtml?alias=" + settingsWrapper.getRootDataverse().getAlias();
     }
 
     public boolean isSignupAllowed() {
@@ -287,28 +280,32 @@ public boolean isRootDataverseThemeDisabled(Dataverse dataverse) {
 
 
     public List<BannerMessage> getBannerMessages() {
-        User user = dataverseSession.getUser();
-        AuthenticatedUser au = null;
-        if (user.isAuthenticated()) {
-            au = (AuthenticatedUser) user;
-        }           
+        if (bannerMessages == null) {
+            bannerMessages = new ArrayList<>();
+
+            User user = dataverseSession.getUser();
+            AuthenticatedUser au = null;
+            if (user.isAuthenticated()) {
+                au = (AuthenticatedUser) user;
+            }           
 
-        if(au == null){
-            bannerMessages = bannerMessageService.findBannerMessages();
-        } else{
-            bannerMessages = bannerMessageService.findBannerMessages(au.getId());
-        } 
+            if  (au == null)    {
+                bannerMessages = bannerMessageService.findBannerMessages();
+            } else  {
+                bannerMessages = bannerMessageService.findBannerMessages(au.getId());
+            } 
 
-        if (!dataverseSession.getDismissedMessages().isEmpty()) {           
-            for (BannerMessage dismissed : dataverseSession.getDismissedMessages()) {
-                Iterator<BannerMessage> itr = bannerMessages.iterator();
-                while (itr.hasNext()) {
-                    BannerMessage test = itr.next();
-                    if (test.equals(dismissed)) {
-                        itr.remove();
+            if (!dataverseSession.getDismissedMessages().isEmpty()) {           
+                for (BannerMessage dismissed : dataverseSession.getDismissedMessages()) {
+                    Iterator<BannerMessage> itr = bannerMessages.iterator();
+                    while (itr.hasNext()) {
+                        BannerMessage test = itr.next();
+                        if (test.equals(dismissed)) {
+                            itr.remove();
+                        }
                     }
-                }
-            }            
+                }            
+            }
         }
 
         return bannerMessages;

src/main/java/edu/harvard/iq/dataverse/DataversePage.java

@@ -25,7 +25,6 @@
 import edu.harvard.iq.dataverse.util.BundleUtil;
 import edu.harvard.iq.dataverse.util.JsfHelper;
 import static edu.harvard.iq.dataverse.util.JsfHelper.JH;
-import edu.harvard.iq.dataverse.util.SystemConfig;
 import java.util.List;
 import javax.ejb.EJB;
 import javax.faces.application.FacesMessage;
@@ -97,8 +96,6 @@ public enum LinkMode {
     ControlledVocabularyValueServiceBean controlledVocabularyValueServiceBean;
     @EJB
     SavedSearchServiceBean savedSearchService;
-    @EJB
-    SystemConfig systemConfig;
     @EJB DataverseRoleServiceBean dataverseRoleServiceBean;
     @Inject
     SearchIncludeFragment searchIncludeFragment;
@@ -318,7 +315,7 @@ public String init() {
                 dataverse = dataverseService.find(this.getId());
             } else {
                 try {
-                    dataverse = dataverseService.findRootDataverse();
+                    dataverse = settingsWrapper.getRootDataverse();
                 } catch (EJBException e) {
                     // @todo handle case with no root dataverse (a fresh installation) with message about using API to create the root 
                     dataverse = null;
@@ -330,6 +327,7 @@ public String init() {
                 return permissionsWrapper.notFound();
             }
             if (!dataverse.isReleased() && !permissionService.on(dataverse).has(Permission.ViewUnpublishedDataverse)) {
+                // the permission lookup above should probably be moved into the permissionsWrapper -- L.A. 5.7
                 return permissionsWrapper.notAuthorized();
             }
 
@@ -340,6 +338,7 @@ public String init() {
             if (dataverse.getOwner() == null) {
                 return  permissionsWrapper.notFound();
             } else if (!permissionService.on(dataverse.getOwner()).has(Permission.AddDataverse)) {
+                // the permission lookup above should probably be moved into the permissionsWrapper -- L.A. 5.7
                 return permissionsWrapper.notAuthorized();            
             }
 
@@ -681,7 +680,7 @@ public String save() {
             if (editMode != null && editMode.equals(EditMode.FEATURED)) {
                 message = BundleUtil.getStringFromBundle("dataverse.feature.update");
             } else {
-                message = (create) ? BundleUtil.getStringFromBundle("dataverse.create.success", Arrays.asList(settingsWrapper.getGuidesBaseUrl(), systemConfig.getGuidesVersion())) : BundleUtil.getStringFromBundle("dataverse.update.success");
+                message = (create) ? BundleUtil.getStringFromBundle("dataverse.create.success", Arrays.asList(settingsWrapper.getGuidesBaseUrl(), settingsWrapper.getGuidesVersion())) : BundleUtil.getStringFromBundle("dataverse.update.success");
             }
             JsfHelper.addSuccessMessage(message);
 
@@ -710,10 +709,13 @@ public String cancel() {
         return "/dataverse.xhtml?alias=" + dataverse.getAlias() + "&faces-redirect=true";
     }
 
-    public boolean isRootDataverse() {
-        return dataverse.getOwner() == null;
+    public boolean isRootDataverse() {        
+        return dataverse == null ? false : dataverse.getOwner() == null;
     }
 
+    // Wondering what this method is for - what would be the situation, where 
+    // we only have the id of the parent dataverse, but it hasn't been instantiated
+    // yet? It doesn't appear like it's being used anywyere. -- L.A. 5.7
     public Dataverse getOwner() {
         return (ownerId != null) ? dataverseService.find(ownerId) : null;
     }

src/main/java/edu/harvard/iq/dataverse/GuestbookResponse.java

@@ -20,6 +20,13 @@
  *
  * @author skraffmiller
  */
+@NamedStoredProcedureQuery(
+        name = "GuestbookResponse.estimateGuestBookResponseTableSize",
+        procedureName = "estimateGuestBookResponseTableSize",
+        parameters = {
+            @StoredProcedureParameter(mode = ParameterMode.OUT, type = Long.class)
+        }
+)
 @Entity
 @Table(indexes = {
         @Index(columnList = "guestbook_id"),

src/main/java/edu/harvard/iq/dataverse/GuestbookResponseServiceBean.java

@@ -33,6 +33,7 @@
 import javax.persistence.EntityManager;
 import javax.persistence.PersistenceContext;
 import javax.persistence.Query;
+import javax.persistence.StoredProcedureQuery;
 import javax.persistence.TypedQuery;
 
 /**
@@ -917,7 +918,34 @@ public Long getCountGuestbookResponsesByDatasetId(Long datasetId) {
     }    
 
     public Long getCountOfAllGuestbookResponses() {
-        // dataset id is null, will return 0        
+        // dataset id is null, will return 0  
+
+        // "SELECT COUNT(*)" is notoriously expensive in PostgresQL for large 
+        // tables. This makes this call fairly expensive for any installation 
+        // with a lot of download/access activity. (This "total download" metrics
+        // is always displayed when the homepage is loaded). 
+        // It is safe to say that no real life user will need this number to be
+        // precise down to the last few digits, and/or withing a second, 
+        // especially once that number is in the millions. The 
+        // solution implemented below relies on estimating the number. It is 
+        // very efficient, but also very PostgresQL specific - hence it is 
+        // defined as a custom database function. 
+        // An alternative solution would be to get the exact number every 
+        // hour or so, and cache it centrally for the rest of the application 
+        // somehow. -- L.A. 5.6
+
+
+        try {        
+            StoredProcedureQuery query = this.em.createNamedStoredProcedureQuery("GuestbookResponse.estimateGuestBookResponseTableSize");
+            query.execute();
+            Long totalCount = (Long) query.getOutputParameterValue(1);
+
+            if (totalCount != null) {
+                return totalCount;
+            }
+        } catch (IllegalArgumentException iae) {
+            // Don't do anything, we'll fall back to using "SELECT COUNT()"
+        }
         Query query = em.createNativeQuery("select count(o.id) from GuestbookResponse  o;");
         return (Long) query.getSingleResult();
     }