Improve MemoryUsage.toString() and constructor error messages for better clarity

XMLWordPrintable

    • Type: Enhancement
    • Resolution: Unresolved
    • Priority: P3
    • None
    • Affects Version/s: None
    • Component/s: core-svc

      ADDITIONAL SYSTEM INFORMATION :
      Generic / MacOS / Java 21

      A DESCRIPTION OF THE PROBLEM :
      **1. MemoryUsage.toString() undefined value display issue**

      When `init` or `max` is `-1` (undefined), `MemoryUsage.toString()` outputs `"-1(-1K)"`, which is confusing. While Javadoc states that `-1` means undefined, the output does not clearly indicate this, making it difficult for users to distinguish undefined values from actual negative values.

      **2. Constructor error message clarity**

      Constructor validation error messages are concise but lack sufficient context for debugging. The current format could be more informative.

      **3. toString() performance issue**

      The `toString()` method uses string concatenation inside `append()` calls, which prevents proper utilization of StringBuilder's benefits.

      ### Steps to Reproduce

      **Issue 1: toString() with undefined values**
      ```java
      MemoryUsage mu = new MemoryUsage(-1, 1024, 2048, -1);
      System.out.println(mu.toString());
      // Current output: "init = -1(-1K) used = 1024(1K) committed = 2048(2K) max = -1(-1K)"
      // Problem: -1(-1K) is confusing and doesn't clearly indicate "undefined"
      ```

      **Issue 2: Constructor error message**
      ```java
      try {
          new MemoryUsage(1024, 512, 256, 1024); // used > committed
      } catch (IllegalArgumentException e) {
          System.out.println(e.getMessage());
          // Current: "used = 512 should be <= committed = 256"
          // Could be more structured and informative
      }
      ```

      **Issue 3: toString() performance**
      ```java
      // Current implementation uses string concatenation in append()
      // This creates intermediate String objects unnecessarily
      MemoryUsage mu = new MemoryUsage(1024, 2048, 4096, 8192);
      String result = mu.toString(); // Inefficient string building
      ```

      ### Expected Result

      **Issue 1:**
      ```
      init = N/A used = 1024(1K) committed = 2048(2K) max = N/A
      ```
      or similar clear indication that the value is undefined.

      **Issue 2:**
      ```
      used (512) must be less than or equal to committed (256)
      ```
      More structured and informative error message.

      **Issue 3:**
      StringBuilder should be used efficiently without intermediate string concatenation.

      ### Actual Result

      **Issue 1:**
      ```
      init = -1(-1K) used = 1024(1K) committed = 2048(2K) max = -1(-1K)
      ```
      The `-1(-1K)` output is confusing and doesn't clearly indicate undefined status.

      **Issue 2:**
      ```
      used = 512 should be <= committed = 256
      ```
      Less structured message format.

      **Issue 3:**
      String concatenation occurs inside `append()` calls, creating unnecessary intermediate objects.

      ### Proposed Solution

      **1. toString() improvements**
      - Display `"N/A"` when `init` or `max` is `-1` (undefined)
      - Refactor to use StringBuilder correctly
      - Maintain existing output format for valid values

      **2. Constructor error message improvements**
      - Use `String.format()` to provide more structured messages
      - Example: `"used (%d) must be less than or equal to committed (%d)"`

      **API Compatibility**: Fully backward compatible. Only output format improvements; no impact on existing code.

      ### Included Change Items

      - **#1**: Improve MemoryUsage.toString() handling of undefined values (-1)
      - **#4**: Improve MemoryUsage.toString() performance (proper StringBuilder usage)
      - **#6**: Improve MemoryUsage constructor error messages

      ### Test Case Code

      **Test for toString() with undefined values:**
      ```java
      /*
       * @test
       * @bug [ISSUE_NUMBER]
       * @summary Test MemoryUsage.toString() with undefined values
       * @run junit ToStringTest
       */

      import org.junit.jupiter.api.Test;
      import static org.junit.jupiter.api.Assertions.*;
      import java.lang.management.MemoryUsage;

      public class ToStringTest {
          @Test
          public void testToStringWithUndefinedInit() {
              MemoryUsage mu = new MemoryUsage(-1, 1024, 2048, 4096);
              String result = mu.toString();
              assertTrue(result.contains("init = N/A"),
                         "Should show N/A for undefined init");
              assertTrue(result.contains("max = 4096"),
                         "Should show max value");
          }
          
          @Test
          public void testToStringWithUndefinedMax() {
              MemoryUsage mu = new MemoryUsage(1024, 2048, 4096, -1);
              String result = mu.toString();
              assertTrue(result.contains("max = N/A"),
                         "Should show N/A for undefined max");
          }
          
          @Test
          public void testToStringWithBothUndefined() {
              MemoryUsage mu = new MemoryUsage(-1, 1024, 2048, -1);
              String result = mu.toString();
              assertTrue(result.contains("init = N/A"),
                         "Should show N/A for undefined init");
              assertTrue(result.contains("max = N/A"),
                         "Should show N/A for undefined max");
          }
      }
      ```

      **Test for improved error messages:**
      ```java
      @Test
      public void testConstructorErrorMessage() {
          try {
              new MemoryUsage(1024, 512, 256, 1024);
              fail("Should throw IllegalArgumentException");
          } catch (IllegalArgumentException e) {
              String msg = e.getMessage();
              assertTrue(msg.contains("used (512)"),
                         "Error message should include used value");
              assertTrue(msg.contains("committed (256)"),
                         "Error message should include committed value");
              assertTrue(msg.contains("must be less than or equal to"),
                         "Error message should be clear and structured");
          }
      }
      ```

      ### Test Strategy

      **Existing Tests**
      - `test/hotspot/jtreg/vmTestbase/nsk/monitoring/MemoryUsage/MemoryUsage/memoryusage001.java` (constructor validation)
      - Existing tests need to be updated to match new message format

      **New Tests to Add**
      - File: `test/jdk/java/lang/management/MemoryUsage/ToStringTest.java`
      - Framework: JUnit 5 (`@run junit`)
      - Test cases:
        - `init = -1, max = -1` (both undefined)
        - `init = -1, max = valid value` (only init undefined)
        - `init = valid value, max = -1` (only max undefined)
        - Very large values (Long.MAX_VALUE)
        - Zero values
        - `used == committed` (boundary case)

      **Test Execution**
      ```bash
      make test TEST="jtreg:test/jdk/java/lang/management/MemoryUsage"
      ```

      ### Impact and Risk Assessment

      - **Backward Compatibility**: Fully compatible. Only output format improvements
      - **Performance Impact**: Minor improvement (StringBuilder usage optimization)
      - **Review Difficulty**: Low. Clear scope and testable changes

      ### First-Time Contributor Suitability

      - Clear problem definition and solution approach
      - Limited scope and easily testable
      - Does not break existing behavior
      - Provides good learning opportunity for JUnit test writing
      - From reviewer perspective: Output clarity improvements are always welcome



            Assignee:
            Unassigned
            Reporter:
            Webbug Group
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

              Created:
              Updated: