CompilerDesignSpring2017

5.0Softwaredesign

Dr.Zoltán Majó

CompilerGroup– JavaHotSpot VirtualMachineOracleCorporation

1

Codequality

§ Whatis“quality”?

§ Allexamshavehighquality.§ Student:Examasksquestionsthatcheckmaterialonthehandout.§ Lecturer:Examshowsthatstudentscanapplymaterialtonew

problem(s).§ Dean(headofstudies):Theresultsfollownormaldistribution.§ Provost:Successfulstudentsaregreatambassadorsforthecollege.

§ Teachingassistant:Examwasmultiplechoice(andthereforecouldbegradedwhilewatchingsoccer).

3

Quality

§ Needtoidentifydimensions§ Needtodefine“low”and“high”marks

§ Withoutclarificationvague

§ Neartermgoal:codereviews§ Bystudents§ Ofstudents’code

§ Why?§ Codereviewsarecommon§ Codereviewshighlyeffectivetoimprovesoftwaresystems

§ Othertechniquesexistaswell4

Codequality

§ Firstfocus:softwaresystemartifact§ Sourcecode§ Documents

§ Externalcharacteristicsofsoftwaresystem§ Visible/noticeableby“user”

§ Internalcharacteristics ofsoftwaresystem§ Primarilyofinteresttodeveloper

§ Otheraspects:process,environment,…

5

Externalcharacteristics

§ Boundarytointernalcharacteristicsnotalwaysstrict

§ Dimensionsarenotalwaysmutuallyexclusive

§ Correctness:freedomfromfaults§ Faultsinspecification§ Faultsindesign§ Faultsinimplementation§ Faultsinexecution

§ Robustness:abilitytohandleinvalidinput§ Usefulerrormessages§ Mayhavetoincludehandlingofhardwarefaults

6

Externalcharacteristics(cont’d)

§ Usability§ Servesitspurpose

§ Adaptability:systemcanbeusedw/omodificationsinotherscenarios§ Interoperateswithothersystemsasdesigned/specified

§ Integrity:prohibitunauthorized/improperaccess

§ Traceability:completenessofaccesstracesand/orlogs

§ Accuracy:resultsacceptablewithregardtoquantitativeinput§ Couldbeconsideredacorrectnessissue

§ Reliability:systemperformsunderstatedconditions

7

Internalcharacteristics

§ Flexibility:systemcanbemodified(withlittleornoeffort)tobeusedforunintendedenvironments

§ Reusability:systemcanbeusedinanotheroperatingenvironment

§ Readability:sourcecodecanbeunderstood

8

Readability

§ Numerousfactorsinfluencereadability§ Consistentstyle(formatting)

§ Useofwhitespace§ Useofindentation§ Toolscanhelp

§ Rulesfornames,namingconventions§ Rulesforcomments§ Useoflanguagefeatures

§ Choiceofprogramminglanguage(s)§ Useofassertions§ Codingconventions

§ Sizeoffunctions§ Useofexceptions

§ File(project)organization/directorystructure9

Readabilityà Understandability

§ Systemstructure§ Reflectshigh-leveldesign§ Modularity

§ Coherenceandcohesion§ Numberofinteractionpointswithothermodules§ Numberofparameters

§ Controlflow§ Useofexceptions§ Control/dataplane

10

Understandability

§ Levelrequiredtocomprehendsystem§ Classes(structs,records)§ Classhierarchies

§ Depth§ Frameworks§ Patterns

11

Internalcharacteristics(cont’d)

§ Maintainability:sourcecodecanbemodifiedtoaddresschangingrequirements§ Improveperformance§ Improveusability§ Addresscorrectnessandsecurityproblems§ Correctdefectsaswellasreacttoenvironmentchanges

§ Testability:Canyouunittestthesystem?§ Howeasyitistoverifythatthesystemmeetsrequirements

13

public void HandleStuff(CORP_DATA inputRec, int crntQtr, EMP_DATA empRec, double estimRevenue, double ytdRevenue, int screenX,int screenY,COLOR_TYPE newColor, COLOR_TYPE prevColor, StatusType status,int expenseType)

{ int i; for (i = 0; i < 100; i++) {

inputRec.revenue[i] = 0; inputRec.expense[i] = MASTER.corpExpense [crntQtr][i];

} MASTER.UpdateCorporateDatabase( empRec );estimRevenue += ytdRevenue * 4.0 / (double) crntQtr;newColor.color = prevColor.color;status.result = Result.SUCCESS;if ( expenseType == 1) {

for (i = 0; i < 12; i++) MASTER.profit[i] = inputRec.revenue[i] - MASTER.expense.type1[i];

} else if ( expenseType == 2) {

MASTER.profit[i] = inputRec.revenue[i] - MASTER.expense.type2[i];}

else if ( expenseType == 3) MASTER.profit[i] = inputRec.revenue[i] - MASTER.expense.type3[i];

}

15

AdaptedfromS.McConnell,CodeComplete(2nd Edition)

16

Comments

§ Internal/externalcharacteristicsmayoverlap§ Correlated,notindependent

§ Oftennofixedruleon“gooddesign”§ Example:sizeoffunctions/methods§ Example:numberofmethodarguments

§ Textstoconsider§ SteveMcConnell:

CodeComplete:APracticalHandbookofSoftwareConstruction,MicrosoftPress,2004.

§ GeorgeA.Miller:TheMagicalNumberSeven,PlusorMinusTwo:SomeLimitsonOurCapacityforProcessingInformation,ThePsychologicalReview,1956,vol.63,pp.81-97(ontheweb)

23

Softwarequality

§ Goal:design&implementationofsoftwaresystemswithdesirableinternalandexternalcharacteristics§ Desirable:dependsoncontextandobjectives§ Combinationsofcharacteristics

§ Stepstoensuresoftwarequality§ Identifyqualitydimensions§ Setpriorities§ Allow(anddiscuss)tradeoffs§ Establishobjectives§ Establishwaystomeasurequality

24

Objectives

§ Manypossibledimensions§ Developmenttimeandcost§ Codelength§ Memoryusage§ Executionspeed§ …

25

Qualityassurance

§ Implicitandexplicitqualityassurancesteps

§ Focushereonexplicitsteps§ Guidelinesforsoftwareconstruction

§ Softwareprocess§ Followprocess

§ Testingstrategy§ Informalreviews§ Formalreviews§ Externalaudits

26

Informalreviews

§ Objective:catcherrorsearly

§ Misuseofinformalreviews§ Assignblame§ Staffevaluation§ Budgetcontrol

§ Informal

27

Informalreviews(cont’d)

§ Kindsofinformalreviews1. Inspection

§ Reviewerslookatcode§ Reviewersmeetdevelopers,askquestions(person-personinteraction)

2. Walk-through§ Presentationofsystem§ Guidetourbydeveloper(s)

3. Codereading§ Feedbackviacomment,review§ Usuallydonebyco-developer

28

Informalreviews

§ Executionofsystem,testcasesmaybepartofthereview§ Emphasisusuallyonunderstandingsourcecode

29

Inspections

§ Participantswithclearlydefinedroles§ Somerolesmutuallyexclusive

30

Roles

§ Moderator§ Keepsreviewgoing§ Mustbetechnicalperson§ Neitherreviewernordeveloper

§ Author§ Developedsoftwaresystemunderreview§ Maystartoutreviewwithanoverview§ Mayanswerfactualquestions(ifasked)

§ Explainparts§ Doesnotdefenddecision

§ Donotargue§ Keyistolisten,codeshouldspeakforitself

31

Roles(cont’d)

§ Reviewer(s)§ Findproblems§ Identifydefects§ Findgoodfeatures§ Findgooduses

§ Scribe§ Takesnotes§ Recorddefects,problems§ Notareviewer§ Mayalsobemoderator

32

Roles(cont’d)

§ Management:Absentfrominspection§ Inspectionisfordevelopers§ Objectiveisnotreviewofdeveloperperformance

§ Numberofparticipants§ Atleastthree:author,reviewer,moderator

§ Moderatorandscribearethesameperson§ Largegroupsmoredifficulttomanagethansmallones

33

Preparation

§ Planning§ Codeordesigndeliveredtomoderator§ Moderatorselectsreviewer(s)

§ Setup§ Overviewbydeveloper§ Danger:developer“directs”reviewers

§ Preparation§ Readingofcode/designdocuments

§ Inspectionmeeting

§ Inspectionreport

34

Followup

§ Reportsstateproblems

§ Moderatorsassigndefectsforrepair§ Maybemanagermapsrequeststodevelopers/authors

§ Anotherinspection§ Dependsonnumberandseverityofdefects

§ Keeplogs,collectdata§ Numberofdefects§ Sizeofsoftwaresystem§ Lengthofmeetings§ Numberofreviewers

35

Codeinspection

§ Well-definedprocess§ Eventhoughprocessisinformal§ “HomeworkC”:Participateininspectionprocess

§ Reviewotherstudents’code§ Haveyourcodereviewed§ Detailslater(nottoday)

§ Processpresentedhere:Apossiblewaytoorganizereviews§ Otherprocessesusedinpractice§ Acasestudy:TheOpenJDK reviewprocess

36

OpenJDK

§ OpenJavaDevelopmentKit§ Freeandopen-sourceimplementationoftheJavaPlatform

§ JavaPlatform§ JavaClassLibrary

§ E.g.,packageslikejava.lang,java.io,java.math§ (Static)Javacompilerjavac

§ TranslateJavaprograms→bytecodeinstructionsforJavaVirtualMachine§ HotSpot JavaVirtualMachine

§ Just-in-timecompilersC1andC2(translatebytecode→nativecode)§ Interpreter(template-based)§ Runtimesystem§ Garbagecollectors

§ …

37

QualityassuranceforOpenJDK

§ “Quality”important§ MillionsofOpenJDK usersworldwide

§ Softwarequalityassurancechallenging§ Largecodebase:7millionsoflinesofcode(Java,C,C++,assembly)§ Contributorsallaroundtheworld

§ Indifferenttimezones§ BothwithinOracleandoutsideOracle(volunteers,SAP,RedHat,Intel)

§ Well-definedcodereviewprocesshelpsensuresoftwarequality§ Reviewsinformal:managementisexcluded§ Otherqualityassurancemethodsusedaswell

§ Onlyreviewprocessdiscussedtoday

38

WorkflowforOpenJDK reviews

§ Step1:Authordevelopsandtestsnewcode§ Usuallyresultsinapatchtotheexistingsystem

§ Step2:Authoruploadspatchtoreviewserver§ cr.openjdk.java.net

§ “Webrev”

39

Webrev

40

Cdiff

41

WorkflowforOpenJDK reviews

§ Step1:Authordevelopsandtestsnewcode§ Usuallyresultsinapatchtotheexistingsystem

§ Step2:Authoruploadspatchtoreviewserver§ cr.openjdk.java.net§ “Webrev”

§ Step3:Authorsendsoutrequestforreview§ Emailtoamailinglist§ E.g.,hotspot-compiler-dev@openjdk.java.net§ Usuallyincludeslinktowebrev,descriptionofproblemandsolution,testing

performed

§ Step4:Reviewersinspectcode,givefeedback§ Ifchangesrequired,authorupdatesandtestscode,continuewithStep2§ OtherwisecontinuewithStep5

§ Step5:Code(“changeset”)pushed§ Eitherbyauthororbyasponsor

42

Changesets

§ Codeannotatedwithawell-formeddescription

§ Example8172844:Assertfailsindeoptimization duetooriginalPCattheendofcodesectionSummary:Changeasserttoacceptendofcodesectionaswell.Reviewed-by:rbackman,kvn,dlongAuthor:zmajo

§ Firstline:UniqueissueID(8172844)toidentifyproblem

§ “Reviewed-by”statementidentifiesreviewers§ Givecredittooftennon-trivialreviewingeffort§ Reviewersareresponsibleforhavocifauthornotavailable

43

OpenJDK reviewprocess

§ Isitcodeinspection,walkthrough,orcodereading?§ Acombinationofallthree§ Ittakeselementsfromeach

§ Walkthrough:Authordescribesproblemandsolution

§ Codereading:Reviewerslookatcodeindependently§ Enforcedalsobygeographicdistribution§ Feedbackgiveninwrittenform(comments)

§ Codeinspection:Well-definedrolesandsteps

44

Roles

Codeinspection(asdescribedearlier)§ Managementnotinvolved

§ Author

§ Reviewer(s)§ Selectedbymoderator

§ Scribe

§ Moderator

OpenJDK reviewprocess

§ Managementnotinvolved

§ Author

§ Reviewer(s)§ Everybodyonmailinglist

§ Mailinglist§ Logallconversations

§ Nomoderation

45

(Lackof)moderation

§ Everybodyregisteredonmailinglistcanreview§ Allmembershavearighttoveto§ Thoughproperjustificationisalwaysrequired§ Dependingonmailinglist,10sor100sofpotentialreviewers

§ Makingallreviewershappyissometimesdifficult§ Andtimeconsuming

§ Upto80%oftimespentonreviewing§ Especiallyifreviewershaveconflictingexpectations§ Morethecasewithcross-componentchanges

§ E.g.,corelibrariesandJITcompilers

§ Sometimes(rarely)consensusisnotreached§ Codethrownaway

46

Otherdifferences

§ SuggestionsforimprovementallowedduringOpenJDK reviews§ Notonlydefectdetection

§ OpenJDK definesahierarchy ofroles§ Contributor

§ Signed“OracleContributorAgreement”§ Author

§ Personwithownusername§ Listedonprojectwebsite

§ Committer§ Allowedtoalsopushcode§ Cansponsorchangesets

§ Reviewer§ Allowedtodecideifcodeis“goodenough”

47

OpenJDK roles

§ Rolesallowfine-grainedaccesscontrol

§ Achieving”Reviewer”status§ Requires32“significant”changesets

§ (Author:2changesets,Committer:8changesets)§ Throughnominationandvotebymembers

§ Allmembershavetherighttoveto

§ “Significant”:anothervaguelydefinedword§ Sometimes:Numberoflinesofcodepushed§ Compilerengineer:1-linechangecantakeweekstodevelop

48

Codereviews

§ Goodwaytoassuresoftwarequality

§ Oftenwell-definedprocess(rulestofollow)§ Helpspeopleinorganizationoperateonsimilarlevelofstandard

§ Defining/followingrules§ Incurs(some)overhead§ Keeptominimum

§ Rulesshouldbeguidedbyobjectivesthatmatter§ Usecommonsensetofindbalance

50